Support for writing structuraly correct ID3v2.3 tags

We don't use the tag footer, extended header or unsynchronization, so we
only need to change the frame size format.

Note that this doesn't write correct ID3v2.3 tags, just tags that have
the correct structure. The content is sill incorrect, unless you add
the right frames manually to the tag instance.

taglib/mpeg/id3v2/id3v2frame.cpp

@@ -484,6 +484,11 @@ TagLib::uint Frame::Header::version() const
   return d->version;
 }
 
+void Frame::Header::setVersion(TagLib::uint version)
+{
+  d->version = version;
+}
+
 bool Frame::Header::tagAlterPreservation() const
 {
   return d->tagAlterPreservation;
@@ -538,7 +543,11 @@ ByteVector Frame::Header::render() const
 {
   ByteVector flags(2, char(0)); // just blank for the moment
 
-  ByteVector v = d->frameID + SynchData::fromUInt(d->frameSize) + flags;
+  ByteVector v = d->frameID +
+    (d->version == 3
+      ? ByteVector::fromUInt(d->frameSize)
+      : SynchData::fromUInt(d->frameSize)) +
+    flags;
 
   return v;
 }

taglib/mpeg/id3v2/id3v2frame.h

@@ -294,11 +294,17 @@ namespace TagLib {
       void setFrameSize(uint size);
 
       /*!
-       * Returns the ID3v2 version of the header (as passed in from the
-       * construction of the header).
+       * Returns the ID3v2 version of the header, as passed in from the
+       * construction of the header or set via setVersion().
        */
       uint version() const;
 
+      /*!
+       * Sets the ID3v2 version of the header, changing has impact on the
+       * correct parsing/rendering of frame data.
+       */
+      void setVersion(uint version);
+
       /*!
        * Returns the size of the frame header in bytes.
        *

taglib/mpeg/id3v2/id3v2header.cpp

@@ -164,7 +164,7 @@ ByteVector Header::render() const
   // add the version number -- we always render a 2.4.0 tag regardless of what
   // the tag originally was.
 
-  v.append(char(4));
+  v.append(char(majorVersion()));
   v.append(char(0));
 
   // Currently we don't actually support writing extended headers, footers or

taglib/mpeg/id3v2/id3v2tag.cpp

@@ -330,6 +330,11 @@ void ID3v2::Tag::removeFrames(const ByteVector &id)
 }
 
 ByteVector ID3v2::Tag::render() const
+{
+  return render(4);
+}
+
+ByteVector ID3v2::Tag::render(int version) const
 {
   // We need to render the "tag data" first so that we have to correct size to
   // render in the tag's header.  The "tag data" -- everything that is included
@@ -343,6 +348,7 @@ ByteVector ID3v2::Tag::render() const
   // Loop through the frames rendering them and adding them to the tagData.
 
   for(FrameList::Iterator it = d->frameList.begin(); it != d->frameList.end(); it++) {
+    (*it)->header()->setVersion(version);
     if((*it)->header()->frameID().size() != 4) {
       debug("A frame of unsupported or unknown type \'"
           + String((*it)->header()->frameID()) + "\' has been discarded");
@@ -364,7 +370,8 @@ ByteVector ID3v2::Tag::render() const
 
   tagData.append(ByteVector(paddingSize, char(0)));
 
-  // Set the tag size.
+  // Set the version and data size.
+  d->header.setMajorVersion(version);
   d->header.setTagSize(tagData.size());
 
   // TODO: This should eventually include d->footer->render().

taglib/mpeg/id3v2/id3v2tag.h

@@ -265,6 +265,15 @@ namespace TagLib {
        */
       ByteVector render() const;
 
+      /*!
+       * Render the tag back to binary data, suitable to be written to disk.
+       *
+       * The \a version parameter specifies the version of the rendered
+       * ID3v2 tag. It can be either 4 or 3.
+       */
+      // BIC: combine with the above method
+      ByteVector render(int version) const;
+
     protected:
       /*!
        * Reads data from the file specified in the constructor.  It does basic

taglib/mpeg/mpegfile.cpp

@@ -139,6 +139,11 @@ bool MPEG::File::save(int tags)
 }
 
 bool MPEG::File::save(int tags, bool stripOthers)
+{
+  return save(tags, stripOthers, 4);
+}
+
+bool MPEG::File::save(int tags, bool stripOthers, int id3v2Version)
 {
   if(tags == NoTags && stripOthers)
     return strip(AllTags);
@@ -174,7 +179,7 @@ bool MPEG::File::save(int tags, bool stripOthers)
       if(!d->hasID3v2)
         d->ID3v2Location = 0;
 
-      insert(ID3v2Tag()->render(), d->ID3v2Location, d->ID3v2OriginalSize);
+      insert(ID3v2Tag()->render(id3v2Version), d->ID3v2Location, d->ID3v2OriginalSize);
 
       d->hasID3v2 = true;
 

taglib/mpeg/mpegfile.h

@@ -161,6 +161,21 @@ namespace TagLib {
       // BIC: combine with the above method
       bool save(int tags, bool stripOthers);
 
+      /*!
+       * Save the file.  This will attempt to save all of the tag types that are
+       * specified by OR-ing together TagTypes values.  The save() method above
+       * uses AllTags.  This returns true if saving was successful.
+       *
+       * If \a stripOthers is true this strips all tags not included in the mask,
+       * but does not modify them in memory, so later calls to save() which make
+       * use of these tags will remain valid.  This also strips empty tags.
+       *
+       * The \a id3v2Version parameter specifies the version of the saved
+       * ID3v2 tag. It can be either 4 or 3.
+       */
+      // BIC: combine with the above method
+      bool save(int tags, bool stripOthers, int id3v2Version);
+
       /*!
        * Returns a pointer to the ID3v2 tag of the file.
        *

tests/test_mpeg.cpp

@@ -2,6 +2,8 @@
 #include <string>
 #include <stdio.h>
 #include <mpegfile.h>
+#include <id3v2tag.h>
+#include "utils.h"
 
 using namespace std;
 using namespace TagLib;
@@ -10,6 +12,8 @@ class TestMPEG : public CppUnit::TestFixture
 {
   CPPUNIT_TEST_SUITE(TestMPEG);
   CPPUNIT_TEST(testVersion2DurationWithXingHeader);
+  CPPUNIT_TEST(testSaveID3v24);
+  CPPUNIT_TEST(testSaveID3v23);
   CPPUNIT_TEST_SUITE_END();
 
 public:
@@ -20,6 +24,38 @@ public:
     CPPUNIT_ASSERT_EQUAL(5387, f.audioProperties()->length());
   }
 
+  void testSaveID3v24()
+  {
+    ScopedFileCopy copy("xing", ".mp3", false);
+    string newname = copy.fileName();
+
+    String xxx = ByteVector(254, 'X');
+    MPEG::File f(newname.c_str());
+    f.tag()->setTitle(xxx);
+    f.tag()->setArtist("Artist A");
+    f.save(MPEG::File::AllTags, true, 4);
+
+    MPEG::File f2(newname.c_str());
+    CPPUNIT_ASSERT_EQUAL(String("Artist A"), f2.tag()->artist());
+    CPPUNIT_ASSERT_EQUAL(xxx, f2.tag()->title());
+  }
+
+  void testSaveID3v23()
+  {
+    ScopedFileCopy copy("xing", ".mp3", false);
+    string newname = copy.fileName();
+
+    String xxx = ByteVector(254, 'X');
+    MPEG::File f(newname.c_str());
+    f.tag()->setTitle(xxx);
+    f.tag()->setArtist("Artist A");
+    f.save(MPEG::File::AllTags, true, 3);
+
+    MPEG::File f2(newname.c_str());
+    CPPUNIT_ASSERT_EQUAL(String("Artist A"), f2.tag()->artist());
+    CPPUNIT_ASSERT_EQUAL(xxx, f2.tag()->title());
+  }
+
 };
 
 CPPUNIT_TEST_SUITE_REGISTRATION(TestMPEG);