| Summary: | Improper RVA2 Frame handling | ||
|---|---|---|---|
| Product: | [Frameworks and Libraries] taglib | Reporter: | Brian Nickel <brian.nickel> |
| Component: | general | Assignee: | Scott Wheeler <wheeler> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | me |
| Priority: | NOR | ||
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Platform: | Gentoo Packages | ||
| OS: | Linux | ||
| Latest Commit: | Version Fixed In: | ||
| Sentry Crash Report: | |||
SVN commit 437115 by wheeler: Fix up the RVA2 handling. I'm still not thrilled with the API, but this should be functional enough to hold over to the next binary incompatible change and has a minimum of differences to the previous API. I did a slightly nasty hack so that the API docs will see just methods with an optional argument, but those are actually two separate methods (for BC). Brian, please feel free to take a look at this prior to 1.4 going out. BUG:107025 M +100 -31 relativevolumeframe.cpp M +61 -11 relativevolumeframe.h The source looks great, but the SVN source has no autogen.sh, nor a configure, so I'm not sure where to begin. make -f Makefile.cvs That generates configure. I also just got sent a couple of files this morning so that I can do some testing myself. Forgot to mention -- that should be at the top level in kdesupport rather than in the TagLib subdirectory. Sorry, but I just got a chance to look at this now, but I found two problems:
In RelativeVolumeFrame::setVolumeAdjustment(float, ChannelType), the volume adjustment is divided by 512: short(adjustment / float(512)) when it should be multiplied: short(adjustment / float(512)), making stored volumes roughly zero.
The second problem is much more severe, as it prevents the loading of the frame in RelativeVolumeFrame::parseFields(const ByteVector &). "pos" is initialised to the position of textDelimiter(String::Latin1) for loading the identification. After that, it searches for a channel type on that same byte rather than the following byte. "pos += 1;" should be placed before "while(pos <= data.size() - 4) {".
Also, outside of this, there really isn't an straight-forward way to create a new frame. I'm doing: new TagLib::ID3v2::RelativeVolumeFrame(TagLib::ByteVector("RVA2\0\0\0\x06\0\0Muine\0", (uint)16)); but it would be nice if you could just do: new TagLib::ID3v2::RelativeVolumeFrame("Muine");
Brian -- I'm going to give this another go in the next week or so. Would you mind writing out a simple test case (ideally including a small mp3 file) and the expected output? That would make it easier to make sure that this isn't broken again in the next release. *** Bug 145298 has been marked as a duplicate of this bug. *** SVN commit 689477 by wheeler:
Fix RVA setting / parsing. Patch from Stephen Booth.
BUG:107025
M +4 -2 relativevolumeframe.cpp
--- trunk/kdesupport/taglib/taglib/mpeg/id3v2/frames/relativevolumeframe.cpp #689476:689477
@@ -130,7 +130,7 @@
void RelativeVolumeFrame::setVolumeAdjustment(float adjustment, ChannelType type)
{
- d->channels[type].volumeAdjustment = short(adjustment / float(512));
+ d->channels[type].volumeAdjustment = short(adjustment * float(512));
}
void RelativeVolumeFrame::setVolumeAdjustment(float adjustment)
@@ -164,8 +164,10 @@
void RelativeVolumeFrame::parseFields(const ByteVector &data)
{
- uint pos = data.find(textDelimiter(String::Latin1));
+ ByteVector delimiter = textDelimiter(String::Latin1);
+ uint pos = data.find(delimiter);
d->identification = String(data.mid(0, pos), String::Latin1);
+ pos += delimiter.size();
// Each channel is at least 4 bytes.
|
Version: 1.3.1 (using KDE KDE 3.4.1DevelKDE 1.2) Installed from: 0Gentoo Packages0Gentoo Packages As it is handled now, the RelativeVolumeFrame handles RVA2 tags wrong and does not look fixable while maintaining API/ABI compatability. The assumption made in the current model is that there is only data for one channel. If you look a the spec, however, it can handle multiple channels: The 'identification' string is used to identify the situation and/or device where this adjustment should apply. The following is then ***repeated*** for every channel This means a frame could be structured: <HEADER> Default $00 $01 $00 $1F $00 $08 $6F $FF $02 $CF $FF (A frame titled "Default" with an adjustment for the Master and both an adjustment and a peak for a Sub) Right now, the API will create a RelativeVolumeFrame with the channelType set to MasterVolume, cutting off the rest of the useful information and losing it.