fix(flac): write a valid last metadata block header

lib/src/writers/flac_writer.dart

@@ -3,100 +3,85 @@ import 'dart:io';
 import 'dart:typed_data';
 
 import 'package:audio_metadata_reader/audio_metadata_reader.dart';
-import 'package:audio_metadata_reader/src/parsers/containers/flac.dart';
 import 'package:audio_metadata_reader/src/utils/bit_manipulator.dart';
 import 'package:audio_metadata_reader/src/writers/base_writer.dart';
 
+typedef _Block = ({int type, Uint8List data});
+
 /// Writer for FLAC metadata blocks (Vorbis comments and pictures).
 class FlacWriter extends BaseMetadataWriter<VorbisMetadata> {
   @override
   void write(File file, VorbisMetadata metadata) {
-    final builder = BytesBuilder();
-
     final reader = file.openSync();
 
-    reader.setPositionSync(0);
+    try {
+      reader.setPositionSync(0);
+      final magic = reader.readSync(4);
 
-    builder.add(reader.readSync(4));
+      // Comment and picture blocks get regenerated, rest carries over
+      final kept = <_Block>[];
+      var isLastBlock = false;
 
-    bool isLastBlock = false;
-    int i = 0;
-    while (!isLastBlock) {
-      if (i == 1) {
-        _writeVorbisComments(builder, metadata);
+      while (!isLastBlock) {
+        final header = reader.readSync(4);
+        isLastBlock = header[0] >> 7 == 1;
+        final type = header[0] & 0x7F;
+        final length = header[3] | header[2] << 8 | header[1] << 16;
 
-        for (var picture in metadata.pictures) {
-          _writePictureBlock(builder, picture);
+        if (type == 4 || type == 6) {
+          reader.setPositionSync(reader.positionSync() + length);
+        } else {
+          kept.add((type: type, data: reader.readSync(length)));
         }
       }
 
-      final block = _parseMetadataBlock(reader, builder, metadata);
-      isLastBlock = block.isLastBlock;
-      i++;
-    }
+      final audioStart = reader.positionSync();
 
-    final rest = reader.lengthSync() - reader.positionSync();
-    builder.add(reader.readSync(rest));
+      // Streaminfo needs to stay first
+      final blocks = <_Block>[];
+      if (kept.isNotEmpty && kept.first.type == 0) {
+        blocks.add(kept.removeAt(0));
+      }
+      blocks.add((type: 4, data: _buildVorbisComments(metadata)));
+      for (final picture in metadata.pictures) {
+        blocks.add((type: 6, data: _buildPictureBlock(picture)));
+      }
+      blocks.addAll(kept);
 
-    reader.closeSync();
-    file.writeAsBytesSync(builder.toBytes());
-  }
+      final builder = BytesBuilder();
+      builder.add(magic);
 
-  MetadataBlockHeader _parseMetadataBlock(
-      RandomAccessFile buffer, BytesBuilder builder, VorbisMetadata metadata) {
-    final bytes = buffer.readSync(4);
-    final byteNumber = bytes[0];
-
-    final MetadataBlockHeader block = (
-      isLastBlock: byteNumber >> 7 == 1, // 0: not last block - 1: last block
-      type: byteNumber & 0x7F, // keep the 7 next bits (0XXXXXXX)
-      length: bytes[3] | bytes[2] << 8 | bytes[1] << 16,
-    );
-
-    // we skip the metadata blocks because we rewrite them
-    // block 4 -> Vorbis comment
-    // block 6 -> picture
-    switch (block.type) {
-      case 3:
-      case 4:
-      case 6:
-        buffer.setPositionSync(buffer.positionSync() + block.length);
-
-        break;
-      default:
-        _writeBlock(
+      // Flag real last block, otherwise the decoder reads into frames
+      for (var i = 0; i < blocks.length; i++) {
+        final block = blocks[i];
+        _writeBlockHeader(
           builder,
-          buffer.readSync(block.length),
           block.type,
-          block.isLastBlock,
+          block.data.length,
+          i == blocks.length - 1,
         );
+        builder.add(block.data);
+      }
 
-        break;
-    }
+      reader.setPositionSync(audioStart);
+      builder.add(reader.readSync(reader.lengthSync() - audioStart));
 
-    return block;
+      file.writeAsBytesSync(builder.toBytes());
+    } finally {
+      reader.closeSync();
+    }
   }
 
   void _writeBlockHeader(
       BytesBuilder builder, int blockType, int length, bool isLastBlock) {
-    int firstByte = isLastBlock ? (1 << 7) : 0;
-    firstByte = (firstByte | blockType);
+    var firstByte = isLastBlock ? (1 << 7) : 0;
+    firstByte = (firstByte | (blockType & 0x7F));
 
     builder.addByte(firstByte);
     builder.add(intToUint24(length));
   }
 
-  void _writeBlock(
-      BytesBuilder builder, Uint8List data, int blockId, bool isLastBlock) {
-    int firstByte = (isLastBlock) ? 255 | blockId : blockId & 255;
-
-    builder.addByte(firstByte);
-    builder.add(intToUint24(data.length));
-
-    builder.add(data);
-  }
-
-  void _writePictureBlock(BytesBuilder builder, Picture picture) {
+  Uint8List _buildPictureBlock(Picture picture) {
     final headerBytes = BytesBuilder();
 
     headerBytes.add(intToUint32(picture.pictureType.index));
@@ -110,59 +95,54 @@ class FlacWriter extends BaseMetadataWriter<VorbisMetadata> {
     headerBytes.add(intToUint32(picture.bytes.length));
     headerBytes.add(picture.bytes);
 
-    final blockLength = headerBytes.length;
-
-    _writeBlockHeader(builder, 6, blockLength, false);
-    builder.add(headerBytes.toBytes());
+    return headerBytes.takeBytes();
   }
 
-  void _writeVorbisComments(BytesBuilder builder, VorbisMetadata metadata) {
+  Uint8List _buildVorbisComments(VorbisMetadata metadata) {
     final mainBuilder = BytesBuilder();
     final commentsBuilder = BytesBuilder();
 
     // vendor length
     // I don't know what to use. The library name? The user selects the name he wants?
     mainBuilder.add(intToUint32LE(0));
 
-    int i = 0;
+    var count = 0;
 
-    void _writeComment(String name, List<String> data) {
-      if (data.isNotEmpty) {
-        for (var d in data) {
-          i++;
-          final toWrite = utf8.encode("$name=$d");
-          commentsBuilder.add(intToUint32LE(toWrite.length));
-          commentsBuilder.add(toWrite);
-        }
+    void writeComment(String name, List<String> data) {
+      for (var d in data) {
+        count++;
+        final toWrite = utf8.encode("$name=$d");
+        commentsBuilder.add(intToUint32LE(toWrite.length));
+        commentsBuilder.add(toWrite);
       }
     }
 
-    _writeComment("TITLE", metadata.title);
-    _writeComment("VERSION", metadata.version);
-    _writeComment("ALBUM", metadata.album);
-    _writeComment(
+    writeComment("TITLE", metadata.title);
+    writeComment("VERSION", metadata.version);
+    writeComment("ALBUM", metadata.album);
+    writeComment(
         "TRACKNUMBER", metadata.trackNumber.map((e) => e.toString()).toList());
-    _writeComment("ARTIST", metadata.artist);
-    _writeComment("PERFORMER", metadata.performer);
-    _writeComment("COPYRIGHT", metadata.copyright);
-    _writeComment("LICENSE", metadata.license);
-    _writeComment("ORGANIZATION", metadata.organization);
-    _writeComment("DESCRIPTION", metadata.description);
-    _writeComment("GENRE", metadata.genres);
-    _writeComment(
+    writeComment("ARTIST", metadata.artist);
+    writeComment("PERFORMER", metadata.performer);
+    writeComment("COPYRIGHT", metadata.copyright);
+    writeComment("LICENSE", metadata.license);
+    writeComment("ORGANIZATION", metadata.organization);
+    writeComment("DESCRIPTION", metadata.description);
+    writeComment("GENRE", metadata.genres);
+    // ISO 8601, slashes are jected by a lot of taggers and Android MediaStore
+    writeComment(
         "DATE",
         metadata.date
             .map((d) =>
-                "${d.year}/${d.month.toString().padLeft(2, "0")}/${d.day.toString().padLeft(2, "0")}")
+                "${d.year.toString().padLeft(4, "0")}-${d.month.toString().padLeft(2, "0")}-${d.day.toString().padLeft(2, "0")}")
             .toList());
-    _writeComment("LOCATION", metadata.location);
-    _writeComment("CONTACT", metadata.contact);
-    _writeComment("ISRC", metadata.isrc);
+    writeComment("LOCATION", metadata.location);
+    writeComment("CONTACT", metadata.contact);
+    writeComment("ISRC", metadata.isrc);
 
-    mainBuilder.add(intToUint32LE(i));
+    mainBuilder.add(intToUint32LE(count));
     mainBuilder.add(commentsBuilder.toBytes());
 
-    _writeBlockHeader(builder, 4, mainBuilder.length, false);
-    builder.add(mainBuilder.takeBytes());
+    return mainBuilder.takeBytes();
   }
 }

test/writers/flac_writer_test.dart

@@ -0,0 +1,85 @@
+import 'dart:io';
+
+import 'package:audio_metadata_reader/audio_metadata_reader.dart';
+import 'package:test/test.dart';
+
+typedef _Header = ({bool isLast, int type, int length});
+
+List<_Header> _readBlocks(File file) {
+  final bytes = file.readAsBytesSync();
+  expect(String.fromCharCodes(bytes.sublist(0, 4)), equals('fLaC'));
+
+  final headers = <_Header>[];
+  var pos = 4;
+  while (true) {
+    final isLast = bytes[pos] >> 7 == 1;
+    final type = bytes[pos] & 0x7F;
+    final length = bytes[pos + 3] | bytes[pos + 2] << 8 | bytes[pos + 1] << 16;
+    headers.add((isLast: isLast, type: type, length: length));
+    pos += 4 + length;
+    if (isLast) break;
+    expect(pos, lessThan(bytes.length), reason: 'ran past end of file');
+  }
+  return headers;
+}
+
+void main() {
+  for (final fixture in ['no_picture.flac', 'picture_last.flac']) {
+    group(fixture, () {
+      late File target;
+
+      setUp(() {
+        final dir = Directory.systemTemp.createTempSync();
+        addTearDown(() => dir.deleteSync(recursive: true));
+        target = File('${dir.path}/track.flac');
+        target.writeAsBytesSync(File('test/flac/$fixture').readAsBytesSync());
+      });
+
+      test('every block header carries a valid type', () {
+        updateMetadata(target, (m) => m.setTitle('Updated title'));
+
+        for (final block in _readBlocks(target)) {
+          expect(block.type, lessThan(7),
+              reason: 'type ${block.type} is reserved');
+          expect(block.type, isNot(equals(127)));
+        }
+      });
+
+      test('exactly one last block flag and it is on the final block', () {
+        updateMetadata(target, (m) => m.setTitle('Updated title'));
+
+        final blocks = _readBlocks(target);
+        expect(blocks.where((b) => b.isLast).length, equals(1));
+        expect(blocks.last.isLast, isTrue);
+      });
+
+      test('repeated writes stay valid', () {
+        for (var i = 0; i < 3; i++) {
+          updateMetadata(target, (m) => m.setTitle('Pass $i'));
+        }
+        expect(readMetadata(target, getImage: false).title, equals('Pass 2'));
+        expect(_readBlocks(target).last.isLast, isTrue);
+      });
+
+      test('date is written as ISO 8601', () {
+        updateMetadata(target, (m) => m.setYear(DateTime(2012, 8, 22)));
+
+        final bytes = target.readAsBytesSync();
+        expect(String.fromCharCodes(bytes).contains('DATE=2012-08-22'), isTrue);
+        expect(
+            String.fromCharCodes(bytes).contains('DATE=2012/08/22'), isFalse);
+      });
+
+      test('blocks other than comment and picture survive', () {
+        final before =
+            _readBlocks(target).where((b) => b.type != 4 && b.type != 6);
+        updateMetadata(target, (m) => m.setTitle('Updated title'));
+        final after =
+            _readBlocks(target).where((b) => b.type != 4 && b.type != 6);
+
+        expect(after.map((b) => b.type).toList(),
+            equals(before.map((b) => b.type).toList()));
+      });
+    });
+  }
+}