From f20e47ae77cdaa2148e3be122bcf329d1648de8a Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Fri, 11 Feb 2022 08:55:42 +0100 Subject: [PATCH] PICARD-2411: Remove Flac seektable if it has no seekpoints A missing seektable is valid. Removing an otherwise empty seektable fixes the issues with software that cannot handle empty seektables. Fully reconstructing a seek table would require decoding the Flac data. --- picard/formats/vorbis.py | 42 +++++++++---------------------------- test/formats/test_vorbis.py | 16 +++++++------- 2 files changed, 18 insertions(+), 40 deletions(-) diff --git a/picard/formats/vorbis.py b/picard/formats/vorbis.py index cebfad42c..4af9d0da5 100644 --- a/picard/formats/vorbis.py +++ b/picard/formats/vorbis.py @@ -29,7 +29,6 @@ import base64 -import math import re import mutagen.flac @@ -104,37 +103,16 @@ def flac_sort_pics_after_tags(metadata_blocks): metadata_blocks.insert(tagindex, pic) -def flac_fix_seektable(file): +def flac_remove_empty_seektable(file): + """Removes an existing but empty seektable from the Flac file. + + Some software has issues with files that contain an empty seek table. Since + no seektable is also valid, remove it. + """ seektable = file.seektable - if not seektable: - seektable = mutagen.flac.SeekTable(None) - if not seektable.seekpoints: - if file.info.total_samples > 0 and math.floor(file.info.length) > 0: - # We try to regenerate the seektable using 4096 samples as a step - # the seekpoint is start pos, end pos, step sample - prev = None - SEEK_SIZE = 4096 - for i in range(file.info.total_samples // SEEK_SIZE): - if prev is None: - prev = 0 - continue - - seektable.seekpoints.append(mutagen.flac.SeekPoint(prev * SEEK_SIZE, i * SEEK_SIZE, SEEK_SIZE)) - prev = i - - if prev is not None: - seektable.seekpoints.append(mutagen.flac.SeekPoint(prev * SEEK_SIZE, file.info.total_samples, file.info.total_samples - prev * SEEK_SIZE)) - elif file.info.total_samples // SEEK_SIZE == 0: - seektable.seekpoints.append(mutagen.flac.SeekPoint(0, file.info.total_samples, file.info.total_samples)) - - if not file.seektable: - file.seektable = seektable - file.metadata_blocks.append(seektable) - - file.seektable.write() - elif file.seektable: - file.metadata_blocks = [b for b in file.metadata_blocks if b != file.seektable] - file.seektable = None + if seektable and not seektable.seekpoints: + file.metadata_blocks = [b for b in file.metadata_blocks if b != file.seektable] + file.seektable = None class VCommentFile(File): @@ -346,7 +324,7 @@ class VCommentFile(File): if is_flac: flac_sort_pics_after_tags(file.metadata_blocks) if config.setting["fix_missing_seekpoints_flac"]: - flac_fix_seektable(file) + flac_remove_empty_seektable(file) kwargs = {} if is_flac and config.setting["remove_id3_from_flac"]: diff --git a/test/formats/test_vorbis.py b/test/formats/test_vorbis.py index 32f999e25..02d8bb0b5 100644 --- a/test/formats/test_vorbis.py +++ b/test/formats/test_vorbis.py @@ -246,30 +246,30 @@ class FLACTest(CommonVorbisTests.VorbisTestCase): self.assertGreater(f.metadata_blocks.index(b), tagindex) self.assertTrue(haspics, "Picture block expected, none found") - @patch.object(vorbis, 'flac_fix_seektable') - def test_setting_fix_missing_seekpoints_flac(self, mock_flac_fix_seektable): + @patch.object(vorbis, 'flac_remove_empty_seektable') + def test_setting_fix_missing_seekpoints_flac(self, mock_flac_remove_empty_seektable): save_metadata(self.filename, Metadata()) - mock_flac_fix_seektable.assert_not_called() + mock_flac_remove_empty_seektable.assert_not_called() self.set_config_values({ 'fix_missing_seekpoints_flac': True }) save_metadata(self.filename, Metadata()) - mock_flac_fix_seektable.assert_called_once() + mock_flac_remove_empty_seektable.assert_called_once() @skipUnlessTestfile - def test_flac_fix_seektable_remove_empty(self): + def test_flac_remove_empty_seektable_remove_empty(self): f = load_raw(self.filename) # Add an empty seek table seektable = SeekTable(None) f.seektable = seektable f.metadata_blocks.append(seektable) # This is a zero length file. The empty seektable should get removed - vorbis.flac_fix_seektable(f) + vorbis.flac_remove_empty_seektable(f) self.assertIsNone(f.seektable) self.assertNotIn(seektable, f.metadata_blocks) @skipUnlessTestfile - def test_flac_fix_seektable_keep_existing(self): + def test_flac_remove_empty_seektable_keep_existing(self): f = load_raw(self.filename) # Add an non-empty seek table seektable = SeekTable(None) @@ -278,7 +278,7 @@ class FLACTest(CommonVorbisTests.VorbisTestCase): f.seektable = seektable f.metadata_blocks.append(seektable) # Existing non-empty seektable should be kept - vorbis.flac_fix_seektable(f) + vorbis.flac_remove_empty_seektable(f) self.assertEqual(seektable, f.seektable) self.assertIn(seektable, f.metadata_blocks) self.assertEqual([seekpoint], f.seektable.seekpoints)