Bug 95545 - RelativeVolumeFrame: ChannelType read/write support not there/broken
Summary: RelativeVolumeFrame: ChannelType read/write support not there/broken
Status: RESOLVED FIXED
Alias: None
Product: taglib
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: Debian testing Linux
: NOR normal
Target Milestone: ---
Assignee: Scott Wheeler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-12-20 22:45 UTC by Felix Berger
Modified: 2006-10-08 21:46 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
corrects reading of relative volume adjustment (1.18 KB, patch)
2005-03-28 11:34 UTC, Felix Berger
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Felix Berger 2004-12-20 22:45:42 UTC
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
Comment 1 Felix Berger 2004-12-27 12:47:02 UTC
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
Comment 2 Scott Wheeler 2005-01-04 05:36:54 UTC
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.

Comment 3 Felix Berger 2005-01-04 13:19:05 UTC
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

Comment 4 Felix Berger 2005-03-28 11:34:39 UTC
Created attachment 10384 [details]
corrects reading of relative volume adjustment
Comment 5 Scott Wheeler 2005-07-21 00:23:42 UTC
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);
Comment 6 Urs Fleisch 2006-10-08 21:43:14 UTC
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;
Comment 7 Urs Fleisch 2006-10-08 21:46:32 UTC
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;