Version: (using KDE KDE 3.3.1) Installed from: Debian testing/unstable Packages OS: Linux Hi, as I could see from normalize's (http://www1.cs.columbia.edu/~cvaill/normalize/) id3 tag implementation, taglib's code seems to be incorrect: void RelativeVolumeFrame::parseFields(const ByteVector &data) { int pos = data.find(textDelimiter(String::Latin1)); d->identification = String(data.mid(0, pos), String::Latin1); // missing increment pos += 1; // at this point the channeltype has to be read in and pos has to be advanced by one d->channelType = (ChannelType)data.at(pos); pos += 1; d->volumeAdjustment = data.mid(pos, 2).toShort(); pos += 2; ... ByteVector RelativeVolumeFrame::renderFields() const { ByteVector data; data.append(d->identification.data(String::Latin1)); data.append(textDelimiter(String::Latin1)); // write out channel type data.append((char)d->channelType); ... I'm not sure if normalize's implementation is correct but taglib's implementation obviously lacks the ChannelType serialization/deserialization code. This simple patch worked for RVA2 tags written by normalize, although I didn't pay much attention for unsignedness of the the channel type char when writing it out due to the lack of a bytevector constructor that takes an unsigned char. Sincerely, Felix Berger
I would like to know what the chances are of getting this into taglib soon. If it's not a big issue and a new release of taglib is in the making anyway, I won't put any effort into trying to implement this in my own taglib client code. Sincerely, Felix Berger
Sorry this wasn't responded to earlier; I tend to disappear for a couple weeks around that time of the year. This code was written by another contributor, so I don't know it well, but I'll take a look at it by this weekend or so. As there don't tend to be a lot of TagLib bugs, often I don't mess with them until I've got enough of them to spend a day or so working on TagLib and then I tend to do a release. At the moment I'm tentatively planning a 1.4 release somewhere a couple weeks before KDE 3.4.
On Tuesday 04 January 2005 05:36, Scott Wheeler wrote: > This code was written by another > contributor, so I don't know it well, but I'll take a look at it by this > weekend or so. That would be cool, thank you. > As there don't tend to be a lot of TagLib bugs, often I don't mess with > them until I've got enough of them to spend a day or so working on TagLib > and then I tend to do a release. At the moment I'm tentatively planning a > 1.4 release somewhere a couple weeks before KDE 3.4. Would KDE 3.4 depend on it then? Cheers, Felix Berger
Created attachment 10384 [details] corrects reading of relative volume adjustment
SVN commit 437091 by wheeler: Fix from Felix Berger to get RVA2 frames a little closer to working. The more complicated fix for #107025 will follow shortly. BUG:95545 M +11 -0 relativevolumeframe.cpp --- trunk/kdesupport/taglib/mpeg/id3v2/frames/relativevolumeframe.cpp #437090:437091 @@ -19,6 +19,8 @@ * USA * ***************************************************************************/ +#include <tdebug.h> + #include "relativevolumeframe.h" using namespace TagLib; @@ -101,9 +103,17 @@ void RelativeVolumeFrame::parseFields(const ByteVector &data) { + if(data.size() < 6) { + debug("A relative volume frame must contain at least 6 bytes."); + return; + } + int pos = data.find(textDelimiter(String::Latin1)); d->identification = String(data.mid(0, pos), String::Latin1); + d->channelType = ChannelType(data[pos]); + pos += 1; + d->volumeAdjustment = data.mid(pos, 2).toShort(); pos += 2; @@ -119,6 +129,7 @@ data.append(d->identification.data(String::Latin1)); data.append(textDelimiter(String::Latin1)); + data.append(char(d->channelType)); data.append(ByteVector::fromShort(d->volumeAdjustment)); data.append(char(d->peakVolume.bitsRepresentingPeak)); data.append(d->peakVolume.peakVolume);
The fix above does not solve the problem. The "pos" variable has to be incremented after parsing the identification. "pos" should also be checked after searching for the identification text delimiter or garbage will be parsed in the best case when a new RVA2 frame is created. The identification cannot be set. I have added a setter and getter. The patch below fixes these three problems. --- taglib/mpeg/id3v2/frames/relativevolumeframe.cpp.orig 2006-09-30 09:20:17.000000000 +0200 +++ taglib/mpeg/id3v2/frames/relativevolumeframe.cpp 2006-09-30 11:10:50.000000000 +0200 @@ -158,18 +158,30 @@ setPeakVolume(peak, MasterVolume); } +String RelativeVolumeFrame::identification() const +{ + return d->identification; +} + +void RelativeVolumeFrame::setIdentification(const String &s) +{ + d->identification = s; +} + //////////////////////////////////////////////////////////////////////////////// // protected members //////////////////////////////////////////////////////////////////////////////// void RelativeVolumeFrame::parseFields(const ByteVector &data) { - uint pos = data.find(textDelimiter(String::Latin1)); + int pos = data.find(textDelimiter(String::Latin1)); + if (pos < 0) return; d->identification = String(data.mid(0, pos), String::Latin1); + pos += 1; // Each channel is at least 4 bytes. - while(pos <= data.size() - 4) { + while(pos <= static_cast<int>(data.size()) - 4) { ChannelType type = ChannelType(data[pos]); --- taglib/mpeg/id3v2/frames/relativevolumeframe.h.orig 2006-09-29 05:52:55.000000000 +0200 +++ taglib/mpeg/id3v2/frames/relativevolumeframe.h 2006-09-30 10:59:31.000000000 +0200 @@ -119,6 +119,20 @@ virtual String toString() const; /*! + * Returns the frame's identification. + * + * \see toString() + * \see setIdentification() + */ + String identification() const; + + /*! + * Sets the identification to \a s. + * + * \see identification() + */ + void setIdentification(const String &s); + + /*! * Returns a list of channels with information currently in the frame. */ List<ChannelType> channels() const;