id3: fix reading/writing v1 and v1.1

Properly read and write ID3v1 and v1.1 tags according to the spec [0].
ID3v1 tags are no longer always read as v1.1, which previously resulted
in wrong track numbers. Writing will use v1 by default, automatically
upgrading to v1.1 if needed.

I am not entirely sure what the WinAmp special-case was for, but the
(also fixed) testcase still passes so ... probably still covered.

[0] https://id3.org/ID3v1

Author: corubba

mutagen/id3/_id3v1.py

@@ -99,17 +99,23 @@ def ParseID3v1(data, v2_version=4, known_frames=None):
     # wrote only the characters available - e.g. "1" or "" - into the
     # year field. To parse those, reduce the size of the year field.
     # Amazingly, "0s" works as a struct format string.
-    unpack_fmt = "3s30s30s30s%ds29sBB" % (len(data) - 124)
+    unpack_fmt = "3s30s30s30s%ds30sB" % (len(data) - 124)
 
     try:
-        tag, title, artist, album, year, comment, track, genre = unpack(
+        tag, title, artist, album, year, comment, genre = unpack(
             unpack_fmt, data)
     except StructError:
         return None
 
     if tag != b"TAG":
         return None
 
+    track = 0
+    if comment[-2] == 0:
+        # treat it as v1.1
+        track = comment[-1]
+        comment = comment[:-2]
+
     def fix(data):
         return data.split(b"\x00")[0].strip().decode('latin1')
 
@@ -149,10 +155,7 @@ def fix(data):
         frames["COMM"] = frame_class["COMM"](
             encoding=0, lang="eng", desc="ID3v1 Comment", text=comment)
 
-    # Don't read a track number if it looks like the comment was
-    # padded with spaces instead of nulls (thanks, WinAmp).
-    if (track and frame_class["TRCK"] and
-            ((track != 32) or (data[-3] == b'\x00'[0]))):
+    if track and frame_class["TRCK"]:
         frames["TRCK"] = TRCK(encoding=0, text=str(track))
     if genre != 255 and frame_class["TCON"]:
         frames["TCON"] = TCON(encoding=0, text=str(genre))
@@ -173,18 +176,26 @@ def MakeID3v1(id3):
         v1[name] = text + (b"\x00" * (30 - len(text)))
 
     if "COMM" in id3:
-        cmnt = id3["COMM"].text[0].encode('latin1', 'replace')[:28]
+        cmnt = id3["COMM"].text[0].encode('latin1', 'replace')[:30]
     else:
         cmnt = b""
-    v1["comment"] = cmnt + (b"\x00" * (29 - len(cmnt)))
+    v1["comment"] = cmnt + (b"\x00" * (30 - len(cmnt)))
 
     if "TRCK" in id3:
         try:
             v1["track"] = bchr(+id3["TRCK"])
         except ValueError:
-            v1["track"] = b"\x00"
+            # nothing useful to add, keep it as v1
+            v1["track"] = b""
+        else:
+            # make it v1.1
+            v1["comment"] = v1["comment"][:-2]
+            v1["track"] = b"\x00" + v1["track"]
     else:
-        v1["track"] = b"\x00"
+        v1["track"] = b""
+
+    if len(v1["comment"]) + len(v1["track"]) != 30:
+        raise ValueError("comment/track field must be 30 bytes long")
 
     if "TCON" in id3:
         try:

tests/test_id3.py

@@ -627,7 +627,7 @@ def test_year(self):
     def test_v1_not_v11(self):
         self.id3["TRCK"] = TRCK(encoding=0, text="32")
         tag = MakeID3v1(self.id3)
-        self.failUnless(32, ParseID3v1(tag)["TRCK"])
+        self.assertEqual("32", ParseID3v1(tag)["TRCK"])
         del self.id3["TRCK"]
         tag = MakeID3v1(self.id3)
         tag = tag[:125] + b'  ' + tag[-1:]
@@ -643,7 +643,7 @@ def test_nulls(self):
         self.assertEquals(b'qrst'.decode('latin1'), tags['TALB'])
 
     def test_nonascii(self):
-        s = u'TAG%(title)30s%(artist)30s%(album)30s%(year)4s%(cmt)29s\x03\x01'
+        s = u'TAG%(title)30s%(artist)30s%(album)30s%(year)4s%(cmt)28s\x00\x03\x01'
         s = s % dict(artist=u'abcd\xe9fg', title=u'hijklmn\xf3p',
                      album=u'qrst\xfcv', cmt=u'wxyz', year=u'1234')
         tags = ParseID3v1(s.encode("latin-1"))
@@ -690,6 +690,63 @@ def test_v1_genre(self):
         v1tag = MakeID3v1(tag)
         self.failUnlessEqual(ParseID3v1(v1tag)["TCON"].genres, ["Pop"])
 
+    def test_v1_comment(self):
+        written = MakeID3v1({"COMM": COMM(encoding=0,
+                                          text="abcdefghijklmnopqrstuvwxyzABCDEFG")})
+        # truncated
+        self.assertEqual(written[97:127], b"abcdefghijklmnopqrstuvwxyzABCD")
+
+        parsed = ParseID3v1(written)
+        self.assertEqual(parsed["COMM"], "abcdefghijklmnopqrstuvwxyzABCD")
+        self.assertFalse("TRCK" in parsed)
+
+    def test_v1_comment_pad(self):
+        written = MakeID3v1({"COMM": COMM(encoding=0, text="   comment")})
+        self.assertEqual(written[97:127], b"   comment" + (b"\x00" * 20))  # padded
+
+        parsed = ParseID3v1(written)
+        self.assertEqual(parsed["COMM"], "comment")  # leading whitespace stripped
+        self.assertFalse("TRCK" in parsed)
+
+    def test_v1_nocomment(self):
+        written = MakeID3v1({})
+        self.assertEqual(written[97:127], b"\x00" * 30)  # all null bytes
+
+        parsed = ParseID3v1(written)
+        self.assertFalse("COMM" in parsed)
+        self.assertFalse("TRCK" in parsed)
+
+    def test_v11_comment(self):
+        written = MakeID3v1({
+            "COMM": COMM(encoding=0, text="abcdefghijklmnopqrstuvwxyzABCDEFG"),
+            "TRCK": TRCK(encoding=0, text="5")
+        })
+        # truncated with separator and track
+        self.assertEqual(written[97:127], b"abcdefghijklmnopqrstuvwxyzAB\x00\x05")
+
+        parsed = ParseID3v1(written)
+        self.assertEqual(parsed["COMM"], "abcdefghijklmnopqrstuvwxyzAB")
+        self.assertEqual(parsed["TRCK"], "5")
+
+    def test_v11_nocomment(self):
+        written = MakeID3v1({"TRCK": TRCK(encoding=0, text="6")})
+        self.assertEqual(written[97:127], (b"\x00" * 29) + b"\x06")
+
+        parsed = ParseID3v1(written)
+        self.assertFalse("COMM" in parsed)
+        self.assertEqual(parsed["TRCK"], "6")
+
+    def test_v11_invalid_track(self):
+        written = MakeID3v1({
+            "COMM": COMM(encoding=0, text="abcdefghijklmnopqrstuvwxyzABCDEFG"),
+            "TRCK": TRCK(encoding=0, text="#7")
+        })
+        self.assertEqual(written[97:127], b"abcdefghijklmnopqrstuvwxyzABCD")  # no track
+
+        parsed = ParseID3v1(written)
+        self.assertEqual(parsed["COMM"], "abcdefghijklmnopqrstuvwxyzABCD")
+        self.assertFalse("TRCK" in parsed)
+
 
 class TestWriteID3v1(TestCase):