Fix saving FLAC files.

This fixes all the issues reported at #622.
This commit is contained in:
Tsuda Kageyu 2015-11-24 12:27:39 +09:00
parent 7b854c5434
commit e6e9ba6094
3 changed files with 200 additions and 64 deletions

View File

@ -46,27 +46,25 @@ using namespace TagLib;
namespace
{
enum { FlacXiphIndex = 0, FlacID3v2Index = 1, FlacID3v1Index = 2 };
enum { MinPaddingLength = 4096 };
enum { LastBlockFlag = 0x80 };
const long MinPaddingLength = 4096;
const long MaxPaddingLegnth = 1024 * 1024;
const char LastBlockFlag = '\x80';
}
class FLAC::File::FilePrivate
{
public:
FilePrivate() :
ID3v2FrameFactory(ID3v2::FrameFactory::instance()),
FilePrivate(const ID3v2::FrameFactory *frameFactory = ID3v2::FrameFactory::instance()) :
ID3v2FrameFactory(frameFactory),
ID3v2Location(-1),
ID3v2OriginalSize(0),
ID3v1Location(-1),
properties(0),
flacStart(0),
streamStart(0),
scanned(false),
hasXiphComment(false),
hasID3v2(false),
hasID3v1(false)
{
}
scanned(false) {}
~FilePrivate()
{
@ -92,10 +90,6 @@ public:
long flacStart;
long streamStart;
bool scanned;
bool hasXiphComment;
bool hasID3v2;
bool hasID3v1;
};
////////////////////////////////////////////////////////////////////////////////
@ -113,9 +107,8 @@ FLAC::File::File(FileName file, bool readProperties, Properties::ReadStyle) :
FLAC::File::File(FileName file, ID3v2::FrameFactory *frameFactory,
bool readProperties, Properties::ReadStyle) :
TagLib::File(file),
d(new FilePrivate())
d(new FilePrivate(frameFactory))
{
d->ID3v2FrameFactory = frameFactory;
if(isOpen())
read(readProperties);
}
@ -123,9 +116,8 @@ FLAC::File::File(FileName file, ID3v2::FrameFactory *frameFactory,
FLAC::File::File(IOStream *stream, ID3v2::FrameFactory *frameFactory,
bool readProperties, Properties::ReadStyle) :
TagLib::File(stream),
d(new FilePrivate())
d(new FilePrivate(frameFactory))
{
d->ID3v2FrameFactory = frameFactory;
if(isOpen())
read(readProperties);
}
@ -214,39 +206,83 @@ bool FLAC::File::save()
data.append(blockData);
}
// Adjust the padding block(s)
// Compute the amount of padding, and append that to data.
// TODO: Should be calculated in offset_t in taglib2.
long originalLength = d->streamStart - d->flacStart;
int paddingLength = originalLength - data.size() - 4;
long paddingLength = originalLength - data.size() - 4;
if(paddingLength <= 0) {
paddingLength = MinPaddingLength;
}
ByteVector padding = ByteVector::fromUInt(paddingLength);
padding.resize(paddingLength + 4);
padding[0] = (char)(FLAC::MetadataBlock::Padding | LastBlockFlag);
data.append(padding);
else {
// Padding won't increase beyond 1% of the file size or 1MB.
long threshold = length() / 100;
threshold = std::max(threshold, MinPaddingLength);
threshold = std::min(threshold, MaxPaddingLegnth);
if(paddingLength > threshold)
paddingLength = MinPaddingLength;
}
ByteVector paddingHeader = ByteVector::fromUInt(paddingLength);
paddingHeader[0] = static_cast<char>(MetadataBlock::Padding | LastBlockFlag);
data.append(paddingHeader);
data.resize(static_cast<TagLib::uint>(data.size() + paddingLength));
// Write the data to the file
insert(data, d->flacStart, originalLength);
d->hasXiphComment = true;
d->streamStart += (data.size() - originalLength);
if(d->ID3v1Location >= 0)
d->ID3v1Location += (data.size() - originalLength);
// Update ID3 tags
if(ID3v2Tag()) {
if(d->hasID3v2) {
if(d->ID3v2Location < d->flacStart)
debug("FLAC::File::save() -- This can't be right -- an ID3v2 tag after the "
"start of the FLAC bytestream? Not writing the ID3v2 tag.");
else
insert(ID3v2Tag()->render(), d->ID3v2Location, d->ID3v2OriginalSize);
if(ID3v2Tag() && !ID3v2Tag()->isEmpty()) {
// ID3v2 tag is not empty. Update the old one or create a new one.
if(d->ID3v2Location < 0)
d->ID3v2Location = 0;
data = ID3v2Tag()->render();
insert(data, d->ID3v2Location, d->ID3v2OriginalSize);
d->flacStart += (data.size() - d->ID3v2OriginalSize);
d->streamStart += (data.size() - d->ID3v2OriginalSize);
if(d->ID3v1Location >= 0)
d->ID3v1Location += (data.size() - d->ID3v2OriginalSize);
d->ID3v2OriginalSize = data.size();
}
else {
// ID3v2 tag is empty. Remove the old one.
if(d->ID3v2Location >= 0) {
removeBlock(d->ID3v2Location, d->ID3v2OriginalSize);
d->flacStart -= d->ID3v2OriginalSize;
d->streamStart -= d->ID3v2OriginalSize;
if(d->ID3v1Location >= 0)
d->ID3v1Location -= d->ID3v2OriginalSize;
d->ID3v2Location = -1;
d->ID3v2OriginalSize = 0;
}
else
insert(ID3v2Tag()->render(), 0, 0);
}
if(ID3v1Tag()) {
if(d->hasID3v1) {
if(ID3v1Tag() && !ID3v1Tag()->isEmpty()) {
// ID3v1 tag is not empty. Update the old one or create a new one.
if(d->ID3v1Location >= 0) {
seek(d->ID3v1Location);
}
else {
@ -255,7 +291,15 @@ bool FLAC::File::save()
}
writeBlock(ID3v1Tag()->render());
d->hasID3v1 = true;
}
else {
// ID3v1 tag is empty. Remove the old one.
if(d->ID3v1Location >= 0) {
truncate(d->ID3v1Location);
d->ID3v1Location = -1;
}
}
return true;
@ -342,17 +386,17 @@ void FLAC::File::removePictures()
bool FLAC::File::hasXiphComment() const
{
return d->hasXiphComment;
return !d->xiphCommentData.isEmpty();
}
bool FLAC::File::hasID3v1Tag() const
{
return d->hasID3v1;
return (d->ID3v1Location >= 0);
}
bool FLAC::File::hasID3v2Tag() const
{
return d->hasID3v2;
return (d->ID3v2Location >= 0);
}
////////////////////////////////////////////////////////////////////////////////
@ -366,25 +410,16 @@ void FLAC::File::read(bool readProperties)
d->ID3v2Location = Utils::findID3v2(this);
if(d->ID3v2Location >= 0) {
d->tag.set(FlacID3v2Index, new ID3v2::Tag(this, d->ID3v2Location, d->ID3v2FrameFactory));
d->ID3v2OriginalSize = ID3v2Tag()->header()->completeTagSize();
if(ID3v2Tag()->header()->tagSize() <= 0)
d->tag.set(FlacID3v2Index, 0);
else
d->hasID3v2 = true;
}
// Look for an ID3v1 tag
d->ID3v1Location = Utils::findID3v1(this);
if(d->ID3v1Location >= 0) {
if(d->ID3v1Location >= 0)
d->tag.set(FlacID3v1Index, new ID3v1::Tag(this, d->ID3v1Location));
d->hasID3v1 = true;
}
// Look for FLAC metadata, including vorbis comments
@ -393,10 +428,10 @@ void FLAC::File::read(bool readProperties)
if(!isValid())
return;
if(d->hasXiphComment)
if(!d->xiphCommentData.isEmpty())
d->tag.set(FlacXiphIndex, new Ogg::XiphComment(d->xiphCommentData));
else
d->tag.set(FlacXiphIndex, new Ogg::XiphComment);
d->tag.set(FlacXiphIndex, new Ogg::XiphComment());
if(readProperties) {
@ -406,10 +441,10 @@ void FLAC::File::read(bool readProperties)
long streamLength;
if(d->hasID3v1)
if(d->ID3v1Location >= 0)
streamLength = d->ID3v1Location - d->streamStart;
else
streamLength = File::length() - d->streamStart;
streamLength = length() - d->streamStart;
d->properties = new Properties(infoData, streamLength);
}
@ -427,7 +462,7 @@ void FLAC::File::scan()
long nextBlockOffset;
if(d->hasID3v2)
if(d->ID3v2Location >= 0)
nextBlockOffset = find("fLaC", d->ID3v2Location + d->ID3v2OriginalSize);
else
nextBlockOffset = find("fLaC");
@ -495,9 +530,8 @@ void FLAC::File::scan()
// Found the vorbis-comment
if(blockType == MetadataBlock::VorbisComment) {
if(!d->hasXiphComment) {
if(d->xiphCommentData.isEmpty()) {
d->xiphCommentData = data;
d->hasXiphComment = true;
}
else {
debug("FLAC::File::scan() -- multiple Vorbis Comment blocks found, using the first one");

View File

@ -155,9 +155,6 @@ namespace TagLib {
* has no XiphComment, one will be constructed from the ID3-tags.
*
* This returns true if the save was successful.
*
* \warning In the current implementation, it's dangerous to call save()
* repeatedly. At worst it will corrupt the file.
*/
virtual bool save();

View File

@ -7,6 +7,7 @@
#include <flacfile.h>
#include <xiphcomment.h>
#include <id3v1tag.h>
#include <id3v2tag.h>
#include <cppunit/extensions/HelperMacros.h>
#include "utils.h"
@ -22,13 +23,19 @@ class TestFLAC : public CppUnit::TestFixture
CPPUNIT_TEST(testAddPicture);
CPPUNIT_TEST(testReplacePicture);
CPPUNIT_TEST(testRemoveAllPictures);
CPPUNIT_TEST(testRepeatedSave);
CPPUNIT_TEST(testRepeatedSave1);
CPPUNIT_TEST(testRepeatedSave2);
CPPUNIT_TEST(testRepeatedSave3);
CPPUNIT_TEST(testSaveMultipleValues);
CPPUNIT_TEST(testDict);
CPPUNIT_TEST(testInvalid);
CPPUNIT_TEST(testAudioProperties);
CPPUNIT_TEST(testZeroSizedPadding);
CPPUNIT_TEST(testZeroSizedPadding1);
CPPUNIT_TEST(testZeroSizedPadding2);
CPPUNIT_TEST(testShrinkPadding);
CPPUNIT_TEST(testSaveID3v1);
CPPUNIT_TEST(testUpdateID3v2);
CPPUNIT_TEST(testEmptyID3v2);
CPPUNIT_TEST_SUITE_END();
public:
@ -185,7 +192,7 @@ public:
}
}
void testRepeatedSave()
void testRepeatedSave1()
{
ScopedFileCopy copy("silence-44-s", ".flac");
string newname = copy.fileName();
@ -206,6 +213,31 @@ public:
}
}
void testRepeatedSave2()
{
ScopedFileCopy copy("no-tags", ".flac");
FLAC::File f(copy.fileName().c_str());
f.ID3v2Tag(true)->setTitle("0123456789");
f.save();
CPPUNIT_ASSERT_EQUAL(5735L, f.length());
f.save();
CPPUNIT_ASSERT_EQUAL(5735L, f.length());
CPPUNIT_ASSERT(f.find("fLaC") >= 0);
}
void testRepeatedSave3()
{
ScopedFileCopy copy("no-tags", ".flac");
FLAC::File f(copy.fileName().c_str());
f.xiphComment()->setTitle(std::string(8 * 1024, 'X').c_str());
f.save();
CPPUNIT_ASSERT_EQUAL(12862L, f.length());
f.save();
CPPUNIT_ASSERT_EQUAL(12862L, f.length());
}
void testSaveMultipleValues()
{
ScopedFileCopy copy("silence-44-s", ".flac");
@ -278,7 +310,7 @@ public:
f.audioProperties()->signature());
}
void testZeroSizedPadding()
void testZeroSizedPadding1()
{
ScopedFileCopy copy("zero-sized-padding", ".flac");
@ -286,6 +318,44 @@ public:
CPPUNIT_ASSERT(f.isValid());
}
void testZeroSizedPadding2()
{
ScopedFileCopy copy("silence-44-s", ".flac");
{
FLAC::File f(copy.fileName().c_str());
f.xiphComment()->setTitle("ABC");
f.save();
}
{
FLAC::File f(copy.fileName().c_str());
f.xiphComment()->setTitle(std::string(3067, 'X').c_str());
f.save();
}
{
FLAC::File f(copy.fileName().c_str());
CPPUNIT_ASSERT(f.isValid());
}
}
void testShrinkPadding()
{
ScopedFileCopy copy("no-tags", ".flac");
{
FLAC::File f(copy.fileName().c_str());
f.xiphComment()->setTitle(std::wstring(128 * 1024, L'X').c_str());
f.save();
CPPUNIT_ASSERT(f.length() > 128 * 1024);
}
{
FLAC::File f(copy.fileName().c_str());
f.xiphComment()->setTitle("0123456789");
f.save();
CPPUNIT_ASSERT(f.length() < 8 * 1024);
}
}
void testSaveID3v1()
{
ScopedFileCopy copy("no-tags", ".flac");
@ -309,6 +379,41 @@ public:
}
}
void testUpdateID3v2()
{
ScopedFileCopy copy("no-tags", ".flac");
{
FLAC::File f(copy.fileName().c_str());
f.ID3v2Tag(true)->setTitle("0123456789");
f.save();
}
{
FLAC::File f(copy.fileName().c_str());
f.ID3v2Tag()->setTitle("ABCDEFGHIJ");
f.save();
}
{
FLAC::File f(copy.fileName().c_str());
CPPUNIT_ASSERT_EQUAL(String("ABCDEFGHIJ"), f.ID3v2Tag()->title());
}
}
void testEmptyID3v2()
{
ScopedFileCopy copy("no-tags", ".flac");
{
FLAC::File f(copy.fileName().c_str());
f.ID3v2Tag(true);
f.save();
}
{
FLAC::File f(copy.fileName().c_str());
CPPUNIT_ASSERT(!f.hasID3v2Tag());
}
}
};
CPPUNIT_TEST_SUITE_REGISTRATION(TestFLAC);