From 48104959b24477516989e6fe51292e733433502d Mon Sep 17 00:00:00 2001 From: Urs Fleisch Date: Sat, 23 Aug 2025 10:39:31 +0200 Subject: [PATCH] Integrate cues, fix element rendering and resizing --- taglib/matroska/ebml/ebmlelement.h | 3 +- taglib/matroska/ebml/ebmlmkcues.cpp | 4 +- taglib/matroska/ebml/ebmlmkcues.h | 6 +- taglib/matroska/ebml/ebmlmkseekhead.cpp | 4 +- taglib/matroska/ebml/ebmlmkseekhead.h | 2 +- taglib/matroska/ebml/ebmlmksegment.cpp | 18 +++++- taglib/matroska/ebml/ebmlmksegment.h | 4 ++ taglib/matroska/matroskaattachments.cpp | 22 +++---- taglib/matroska/matroskaattachments.h | 4 +- taglib/matroska/matroskacues.cpp | 82 +++++++++++++++++++------ taglib/matroska/matroskacues.h | 40 ++++++------ taglib/matroska/matroskaelement.cpp | 44 ++++++++++++- taglib/matroska/matroskaelement.h | 10 ++- taglib/matroska/matroskafile.cpp | 64 ++++++++++++++++--- taglib/matroska/matroskafile.h | 6 +- taglib/matroska/matroskaseekhead.cpp | 65 +++++++++++--------- taglib/matroska/matroskaseekhead.h | 8 +-- taglib/matroska/matroskasegment.cpp | 23 +++++-- taglib/matroska/matroskasegment.h | 2 + taglib/matroska/matroskatag.cpp | 28 +++++---- taglib/matroska/matroskatag.h | 2 +- 21 files changed, 316 insertions(+), 125 deletions(-) diff --git a/taglib/matroska/ebml/ebmlelement.h b/taglib/matroska/ebml/ebmlelement.h index 8f6a3fe8..84639c8f 100644 --- a/taglib/matroska/ebml/ebmlelement.h +++ b/taglib/matroska/ebml/ebmlelement.h @@ -130,6 +130,7 @@ namespace TagLib::EBML { class MkTags; class MkAttachments; class MkSeekHead; + class MkCues; class VoidElement; template @@ -157,7 +158,7 @@ namespace TagLib::EBML { template <> struct GetElementTypeById { using type = MasterElement; }; template <> struct GetElementTypeById { using type = MasterElement; }; template <> struct GetElementTypeById { using type = MasterElement; }; - template <> struct GetElementTypeById { using type = MasterElement; }; + template <> struct GetElementTypeById { using type = MkCues; }; template <> struct GetElementTypeById { using type = UTF8StringElement; }; template <> struct GetElementTypeById { using type = UTF8StringElement; }; template <> struct GetElementTypeById { using type = UTF8StringElement; }; diff --git a/taglib/matroska/ebml/ebmlmkcues.cpp b/taglib/matroska/ebml/ebmlmkcues.cpp index a97f7dd2..4ba50b3e 100644 --- a/taglib/matroska/ebml/ebmlmkcues.cpp +++ b/taglib/matroska/ebml/ebmlmkcues.cpp @@ -24,9 +24,9 @@ using namespace TagLib; -Matroska::Cues *EBML::MkCues::parse() +std::unique_ptr EBML::MkCues::parse(offset_t segmentDataOffset) { - auto cues = new Matroska::Cues(); + auto cues = std::make_unique(segmentDataOffset); cues->setOffset(offset); cues->setSize(getSize()); cues->setID(static_cast(id)); diff --git a/taglib/matroska/ebml/ebmlmkcues.h b/taglib/matroska/ebml/ebmlmkcues.h index 79530513..edb2b428 100644 --- a/taglib/matroska/ebml/ebmlmkcues.h +++ b/taglib/matroska/ebml/ebmlmkcues.h @@ -39,12 +39,16 @@ namespace TagLib { MasterElement(Id::MkCues, sizeLength, dataSize, offset) { } + MkCues(Id, int sizeLength, offset_t dataSize, offset_t offset) : + MasterElement(Id::MkCues, sizeLength, dataSize, offset) + { + } MkCues() : MasterElement(Id::MkCues, 0, 0, 0) { } - Matroska::Cues *parse(); + std::unique_ptr parse(offset_t segmentDataOffset); }; } } diff --git a/taglib/matroska/ebml/ebmlmkseekhead.cpp b/taglib/matroska/ebml/ebmlmkseekhead.cpp index 20ae34bf..dccca140 100644 --- a/taglib/matroska/ebml/ebmlmkseekhead.cpp +++ b/taglib/matroska/ebml/ebmlmkseekhead.cpp @@ -25,9 +25,9 @@ using namespace TagLib; -std::unique_ptr EBML::MkSeekHead::parse() +std::unique_ptr EBML::MkSeekHead::parse(offset_t segmentDataOffset) { - auto seekHead = std::make_unique(); + auto seekHead = std::make_unique(segmentDataOffset); seekHead->setOffset(offset); seekHead->setSize(getSize() + padding); diff --git a/taglib/matroska/ebml/ebmlmkseekhead.h b/taglib/matroska/ebml/ebmlmkseekhead.h index de405c6a..da7b3071 100644 --- a/taglib/matroska/ebml/ebmlmkseekhead.h +++ b/taglib/matroska/ebml/ebmlmkseekhead.h @@ -46,7 +46,7 @@ namespace TagLib { { } - std::unique_ptr parse(); + std::unique_ptr parse(offset_t segmentDataOffset); }; } } diff --git a/taglib/matroska/ebml/ebmlmksegment.cpp b/taglib/matroska/ebml/ebmlmksegment.cpp index 887ca5d9..c8e068e2 100644 --- a/taglib/matroska/ebml/ebmlmksegment.cpp +++ b/taglib/matroska/ebml/ebmlmksegment.cpp @@ -28,6 +28,7 @@ #include "matroskafile.h" #include "matroskatag.h" #include "matroskaattachments.h" +#include "matroskacues.h" #include "matroskaseekhead.h" #include "matroskasegment.h" @@ -35,6 +36,11 @@ using namespace TagLib; EBML::MkSegment::~MkSegment() = default; +offset_t EBML::MkSegment::segmentDataOffset() const +{ + return offset + idSize(id) + sizeLength; +} + bool EBML::MkSegment::read(File &file) { offset_t maxOffset = file.tell() + dataSize; @@ -49,6 +55,11 @@ bool EBML::MkSegment::read(File &file) if(!seekHead->read(file)) return false; } + else if(id == Id::MkCues) { + cues = element_cast(std::move(element)); + if(!cues->read(file)) + return false; + } else if(id == Id::MkInfo) { info = element_cast(std::move(element)); if(!info->read(file)) @@ -94,7 +105,12 @@ std::unique_ptr EBML::MkSegment::parseAttachments() std::unique_ptr EBML::MkSegment::parseSeekHead() { - return seekHead ? seekHead->parse() : nullptr; + return seekHead ? seekHead->parse(segmentDataOffset()) : nullptr; +} + +std::unique_ptr EBML::MkSegment::parseCues() +{ + return cues ? cues->parse(segmentDataOffset()) : nullptr; } std::unique_ptr EBML::MkSegment::parseSegment() diff --git a/taglib/matroska/ebml/ebmlmksegment.h b/taglib/matroska/ebml/ebmlmksegment.h index e913f36c..b2c3e95b 100644 --- a/taglib/matroska/ebml/ebmlmksegment.h +++ b/taglib/matroska/ebml/ebmlmksegment.h @@ -26,6 +26,7 @@ #include "ebmlmktags.h" #include "ebmlmkattachments.h" #include "ebmlmkseekhead.h" +#include "ebmlmkcues.h" #include "ebmlmkinfo.h" #include "ebmlmktracks.h" #include "taglib.h" @@ -50,10 +51,12 @@ namespace TagLib { { } ~MkSegment() override; + offset_t segmentDataOffset() const; bool read(File &file) override; std::unique_ptr parseTag(); std::unique_ptr parseAttachments(); std::unique_ptr parseSeekHead(); + std::unique_ptr parseCues(); std::unique_ptr parseSegment(); void parseInfo(Matroska::Properties *properties); void parseTracks(Matroska::Properties *properties); @@ -62,6 +65,7 @@ namespace TagLib { std::unique_ptr tags; std::unique_ptr attachments; std::unique_ptr seekHead; + std::unique_ptr cues; std::unique_ptr info; std::unique_ptr tracks; }; diff --git a/taglib/matroska/matroskaattachments.cpp b/taglib/matroska/matroskaattachments.cpp index b11eafb2..2a0ad9d7 100644 --- a/taglib/matroska/matroskaattachments.cpp +++ b/taglib/matroska/matroskaattachments.cpp @@ -56,6 +56,7 @@ Matroska::Attachments::~Attachments() = default; void Matroska::Attachments::addAttachedFile(const AttachedFile& file) { d->files.append(file); + setNeedsRender(true); } void Matroska::Attachments::removeAttachedFile(unsigned long long uid) @@ -66,12 +67,14 @@ void Matroska::Attachments::removeAttachedFile(unsigned long long uid) }); if(it != d->files.end()) { d->files.erase(it); + setNeedsRender(true); } } void Matroska::Attachments::clear() { d->files.clear(); + setNeedsRender(true); } const Matroska::Attachments::AttachedFileList &Matroska::Attachments::attachedFileList() const @@ -85,11 +88,17 @@ const Matroska::Attachments::AttachedFileList &Matroska::Attachments::attachedFi Matroska::Attachments::AttachedFileList &Matroska::Attachments::attachedFiles() { + setNeedsRender(true); return d->files; } -bool Matroska::Attachments::render() +ByteVector Matroska::Attachments::renderInternal() { + if(d->files.isEmpty()) { + // Avoid writing an Attachments element without AttachedFile element. + return {}; + } + EBML::MkAttachments attachments; for(const auto &attachedFile : std::as_const(d->files)) { auto attachedFileElement = EBML::make_unique_element(); @@ -127,14 +136,5 @@ bool Matroska::Attachments::render() attachments.appendElement(std::move(attachedFileElement)); } - - auto beforeSize = size(); - auto data = attachments.render(); - auto afterSize = data.size(); - if(beforeSize != afterSize) { - if(!emitSizeChanged(afterSize - beforeSize)) - return false; - } - setData(data); - return true; + return attachments.render(); } diff --git a/taglib/matroska/matroskaattachments.h b/taglib/matroska/matroskaattachments.h index b5ae7d1e..d7296057 100644 --- a/taglib/matroska/matroskaattachments.h +++ b/taglib/matroska/matroskaattachments.h @@ -47,7 +47,7 @@ namespace TagLib { Attachments(); //! Destroy attachements. - ~Attachments(); + virtual ~Attachments(); //! Add an attached file. void addAttachedFile(const AttachedFile &file); @@ -67,7 +67,7 @@ namespace TagLib { class AttachmentsPrivate; // private Element implementation - bool render() override; + ByteVector renderInternal() override; AttachedFileList &attachedFiles(); TAGLIB_MSVC_SUPPRESS_WARNING_NEEDS_TO_HAVE_DLL_INTERFACE diff --git a/taglib/matroska/matroskacues.cpp b/taglib/matroska/matroskacues.cpp index a064ae51..d4390a5c 100644 --- a/taglib/matroska/matroskacues.cpp +++ b/taglib/matroska/matroskacues.cpp @@ -29,14 +29,18 @@ using namespace TagLib; -Matroska::Cues::Cues() : - Element(static_cast(EBML::Element::Id::MkCues)) +Matroska::Cues::Cues(offset_t segmentDataOffset) : + Element(static_cast(EBML::Element::Id::MkCues)), + segmentDataOffset(segmentDataOffset) { + setNeedsRender(false); } ByteVector Matroska::Cues::renderInternal() { + auto beforeSize = sizeRenderedOrWritten(); EBML::MkCues cues; + cues.setMinRenderSize(beforeSize); for(const auto &cuePoint : cuePoints) { auto cuePointElement = EBML::make_unique_element(); auto timestamp = EBML::make_unique_element(); @@ -57,7 +61,33 @@ ByteVector Matroska::Cues::renderInternal() clusterPosition->setValue(cueTrack->getClusterPosition()); cueTrackElement->appendElement(std::move(clusterPosition)); - // Todo - other elements + // Relative position, optional + if(cueTrack->getRelativePosition().has_value()) { + auto relativePosition = EBML::make_unique_element(); + relativePosition->setValue(cueTrack->getRelativePosition().value()); + cueTrackElement->appendElement(std::move(relativePosition)); + } + + // Duration, optional + if(cueTrack->getDuration().has_value()) { + auto duration = EBML::make_unique_element(); + duration->setValue(cueTrack->getDuration().value()); + cueTrackElement->appendElement(std::move(duration)); + } + + // Block number, optional + if(cueTrack->getBlockNumber().has_value()) { + auto blockNumber = EBML::make_unique_element(); + blockNumber->setValue(cueTrack->getBlockNumber().value()); + cueTrackElement->appendElement(std::move(blockNumber)); + } + + // Codec state, not in version 1 + if(cueTrack->getCodecState().has_value()) { + auto codecState = EBML::make_unique_element(); + codecState->setValue(cueTrack->getCodecState().value()); + cueTrackElement->appendElement(std::move(codecState)); + } // Reference times auto referenceTimes = cueTrack->referenceTimes(); @@ -72,29 +102,33 @@ ByteVector Matroska::Cues::renderInternal() } cuePointElement->appendElement(std::move(cueTrackElement)); } + cues.appendElement(std::move(cuePointElement)); } return cues.render(); } -bool Matroska::Cues::render() +void Matroska::Cues::write(TagLib::File& file) { - if(!needsRender) - return true; - - setData(renderInternal()); - needsRender = false; - return true; + if(!data().isEmpty()) + Element::write(file); } bool Matroska::Cues::sizeChanged(Element &caller, offset_t delta) { - offset_t offset = caller.offset(); - for(auto &cuePoint : cuePoints) - needsRender |= cuePoint->adjustOffset(offset, delta); + // Adjust own offset + if(!Element::sizeChanged(caller, delta)) + return false; + + offset_t offset = caller.offset() - segmentDataOffset; + for(auto &cuePoint : cuePoints) { + if(cuePoint->adjustOffset(offset, delta)) { + setNeedsRender(true); + } + } return true; } -bool Matroska::Cues::isValid(TagLib::File &file, offset_t segmentDataOffset) const +bool Matroska::Cues::isValid(TagLib::File &file) const { for(const auto &cuePoint : cuePoints) { if(!cuePoint->isValid(file, segmentDataOffset)) @@ -150,8 +184,8 @@ bool Matroska::CueTrack::isValid(TagLib::File &file, offset_t segmentDataOffset) debug("No cluster found at position"); return false; } - if(codecState) { - file.seek(segmentDataOffset + codecState); + if(codecState.has_value() && codecState.value() != 0) { + file.seek(segmentDataOffset + codecState.value()); if(EBML::Element::readId(file) != static_cast(EBML::Element::Id::MkCodecState)) { debug("No codec state found at position"); return false; @@ -160,8 +194,18 @@ bool Matroska::CueTrack::isValid(TagLib::File &file, offset_t segmentDataOffset) return true; } -bool Matroska::CueTrack::adjustOffset(offset_t, offset_t) +bool Matroska::CueTrack::adjustOffset(offset_t offset, offset_t delta) { - // TODO implement - return false; + bool ret = false; + if(clusterPosition > offset) { + clusterPosition += delta; + ret = true; + } + offset_t codecStateValue; + if(codecState.has_value() && (codecStateValue = codecState.value()) != 0 && + codecStateValue > offset) { + codecState = codecStateValue + delta; + ret = true; + } + return ret; } diff --git a/taglib/matroska/matroskacues.h b/taglib/matroska/matroskacues.h index 192599f3..ee971d04 100644 --- a/taglib/matroska/matroskacues.h +++ b/taglib/matroska/matroskacues.h @@ -22,6 +22,8 @@ #define TAGLIB_MATROSKACUES_H #ifndef DO_NOT_DOCUMENT +#include + #include "tlist.h" #include "matroskaelement.h" @@ -38,20 +40,20 @@ namespace TagLib { { public: using CuePointList = std::list>; - Cues(); + explicit Cues(offset_t segmentDataOffset); ~Cues() override = default; - bool isValid(File &file, offset_t segmentDataOffset) const; + bool isValid(TagLib::File &file) const; void addCuePoint(std::unique_ptr &&cuePoint); const CuePointList &cuePointList() { return cuePoints; } bool sizeChanged(Element &caller, offset_t delta) override; - bool render() override; + void write(TagLib::File &file) override; private: friend class EBML::MkCues; - ByteVector renderInternal(); - bool needsRender = false; + ByteVector renderInternal() override; CuePointList cuePoints; + const offset_t segmentDataOffset; }; class CuePoint @@ -61,7 +63,7 @@ namespace TagLib { using Time = unsigned long long; CuePoint(); ~CuePoint() = default; - bool isValid(File &file, offset_t segmentDataOffset) const; + bool isValid(TagLib::File &file, offset_t segmentDataOffset) const; void addCueTrack(std::unique_ptr &&cueTrack); const CueTrackList &cueTrackList() const { return cueTracks; } void setTime(Time time) { this->time = time; } @@ -79,19 +81,19 @@ namespace TagLib { using ReferenceTimeList = List; CueTrack() = default; ~CueTrack() = default; - bool isValid(File &file, offset_t segmentDataOffset) const; + bool isValid(TagLib::File &file, offset_t segmentDataOffset) const; void setTrackNumber(unsigned long long trackNumber) { this->trackNumber = trackNumber; } unsigned long long getTrackNumber() const { return trackNumber; } void setClusterPosition(offset_t clusterPosition) { this->clusterPosition = clusterPosition; } offset_t getClusterPosition() const { return clusterPosition; } - void setRelativePosition(offset_t relativePosition) { this->relativePosition = relativePosition; } - offset_t getRelativePosition() const { return relativePosition; } - void setCodecState(offset_t codecState) { this->codecState = codecState; } - offset_t getCodecState() const { return codecState; } - void setBlockNumber(unsigned long long blockNumber) { this->blockNumber = blockNumber; } - unsigned long long getBlockNumber() const { return blockNumber; } - void setDuration(unsigned long long duration) { this->duration = duration; } - unsigned long long getDuration() const { return duration; } + void setRelativePosition(std::optional relativePosition) { this->relativePosition = relativePosition; } + std::optional getRelativePosition() const { return relativePosition; } + void setCodecState(std::optional codecState) { this->codecState = codecState; } + std::optional getCodecState() const { return codecState; } + void setBlockNumber(std::optional blockNumber) { this->blockNumber = blockNumber; } + std::optional getBlockNumber() const { return blockNumber; } + void setDuration(std::optional duration) { this->duration = duration; } + std::optional getDuration() const { return duration; } void addReferenceTime(unsigned long long refTime) { refTimes.append(refTime); } const ReferenceTimeList &referenceTimes() const { return refTimes; } bool adjustOffset(offset_t offset, offset_t delta); @@ -99,10 +101,10 @@ namespace TagLib { private: unsigned long long trackNumber = 0; offset_t clusterPosition = 0; - offset_t relativePosition = 0; - unsigned long long blockNumber = 0; - unsigned long long duration = 0; - offset_t codecState = 0; + std::optional relativePosition; + std::optional blockNumber; + std::optional duration; + std::optional codecState; ReferenceTimeList refTimes; }; } diff --git a/taglib/matroska/matroskaelement.cpp b/taglib/matroska/matroskaelement.cpp index 3fb65d6d..2bd5c076 100644 --- a/taglib/matroska/matroskaelement.cpp +++ b/taglib/matroska/matroskaelement.cpp @@ -38,6 +38,10 @@ public: ByteVector data; List sizeListeners; List offsetListeners; + // The default write() implementation will delete an unrendered element, + // therefore rendering is required by default and needs to be explicitly set + // using setNeedsRender(false) together with overriding the write() method. + bool needsRender = true; }; Matroska::Element::Element(ID id) : @@ -112,6 +116,35 @@ void Matroska::Element::setID(ID id) e->id = id; } +bool Matroska::Element::render() +{ + if(!needsRender()) + return true; + + auto beforeSize = sizeRenderedOrWritten(); + auto data = renderInternal(); + setNeedsRender(false); + auto afterSize = data.size(); + if(afterSize != beforeSize) { + if(!emitSizeChanged(afterSize - beforeSize)) { + return false; + } + } + + setData(data); + return true; +} + +void Matroska::Element::setNeedsRender(bool needsRender) +{ + e->needsRender = needsRender; +} + +bool Matroska::Element::needsRender() const +{ + return e->needsRender; +} + bool Matroska::Element::emitSizeChanged(offset_t delta) { for(auto element : e->sizeListeners) { @@ -132,13 +165,22 @@ bool Matroska::Element::emitOffsetChanged(offset_t delta) bool Matroska::Element::sizeChanged(Element &caller, offset_t delta) { - if(caller.offset() < e->offset) { + // The equal case is needed when multiple new elements are added + // (e.g. Attachments and Tags), they will start with the same offset + // are updated via size change handling. + if(caller.offset() <= e->offset && caller.id() != e->id) { e->offset += delta; //return emitOffsetChanged(delta); } return true; } +offset_t Matroska::Element::sizeRenderedOrWritten() const +{ + offset_t dataSize = e->data.size(); + return dataSize != 0 ? dataSize : e->size; +} + bool Matroska::Element::offsetChanged(Element &, offset_t) { // Most elements don't need to handle this diff --git a/taglib/matroska/matroskaelement.h b/taglib/matroska/matroskaelement.h index 9c1f19c3..35e76393 100644 --- a/taglib/matroska/matroskaelement.h +++ b/taglib/matroska/matroskaelement.h @@ -45,8 +45,9 @@ namespace TagLib { void adjustOffset(offset_t delta); void setSize(offset_t size); void setID(ID id); - //virtual ByteVector render() = 0; - virtual bool render() = 0; + virtual bool render(); + void setNeedsRender(bool needsRender); + bool needsRender() const; void setData(const ByteVector &data); const ByteVector &data() const; virtual void write(TagLib::File &file); @@ -60,7 +61,12 @@ namespace TagLib { virtual bool offsetChanged(Element &caller, offset_t delta); virtual bool sizeChanged(Element &caller, offset_t delta); + protected: + offset_t sizeRenderedOrWritten() const; + private: + virtual ByteVector renderInternal() = 0; + class ElementPrivate; TAGLIB_MSVC_SUPPRESS_WARNING_NEEDS_TO_HAVE_DLL_INTERFACE std::unique_ptr e; diff --git a/taglib/matroska/matroskafile.cpp b/taglib/matroska/matroskafile.cpp index 3ee1065f..29e4d9db 100644 --- a/taglib/matroska/matroskafile.cpp +++ b/taglib/matroska/matroskafile.cpp @@ -23,6 +23,7 @@ #include "matroskaattachments.h" #include "matroskaattachedfile.h" #include "matroskaseekhead.h" +#include "matroskacues.h" #include "matroskasegment.h" #include "ebmlutils.h" #include "ebmlelement.h" @@ -48,6 +49,7 @@ public: std::unique_ptr tag; std::unique_ptr attachments; std::unique_ptr seekHead; + std::unique_ptr cues; std::unique_ptr segment; std::unique_ptr properties; }; @@ -260,7 +262,7 @@ Matroska::Attachments *Matroska::File::attachments(bool create) const return d->attachments.get(); } -void Matroska::File::read(bool readProperties, Properties::ReadStyle) +void Matroska::File::read(bool readProperties, Properties::ReadStyle readStyle) { offset_t fileLength = length(); @@ -295,16 +297,23 @@ void Matroska::File::read(bool readProperties, Properties::ReadStyle) // Parse the elements d->segment = segment->parseSegment(); d->seekHead = segment->parseSeekHead(); + d->cues = segment->parseCues(); d->tag = segment->parseTag(); d->attachments = segment->parseAttachments(); - setValid(true); - if(readProperties) { d->properties = std::make_unique(this); segment->parseInfo(d->properties.get()); segment->parseTracks(d->properties.get()); } + + if(readStyle == AudioProperties::Accurate && + ((d->seekHead && !d->seekHead->isValid(*this)) || + (d->cues && !d->cues->isValid(*this)))) { + setValid(false); + return; + } + setValid(true); } bool Matroska::File::save() @@ -318,6 +327,19 @@ bool Matroska::File::save() return false; } + // Do not create new attachments or tags and corresponding seek head entries + // if only empty objects were created. + if(d->attachments && d->attachments->attachedFileList().isEmpty() && + d->attachments->size() == 0 && d->attachments->offset() == 0 && + d->attachments->data().isEmpty()) { + d->attachments.reset(); + } + if(d->tag && d->tag->isEmpty() && + d->tag->size() == 0 && d->tag->offset() == 0 && + d->tag->data().isEmpty()) { + d->tag.reset(); + } + List renderList; List newElements; @@ -345,7 +367,7 @@ bool Matroska::File::save() newElements.append(element); } } - if(renderList.isEmpty()) + if(renderList.isEmpty() && newElements.isEmpty()) return true; auto sortAscending = [](const auto a, const auto b) { return a->offset() < b->offset(); }; @@ -364,21 +386,45 @@ bool Matroska::File::save() for(auto it = renderList.begin(); it != renderList.end(); ++it) { for(auto it2 = std::next(it); it2 != renderList.end(); ++it2) (*it)->addSizeListener(*it2); + if(d->cues) + (*it)->addSizeListener(d->cues.get()); if(d->seekHead) (*it)->addSizeListener(d->seekHead.get()); (*it)->addSizeListener(d->segment.get()); } + if(d->cues) { + renderList.append(d->cues.get()); + d->cues->addSizeListeners(renderList); + if(d->seekHead) { + d->cues->addSizeListener(d->seekHead.get()); + } + d->cues->addSizeListener(d->segment.get()); + } if(d->seekHead) { - d->seekHead->addSizeListeners(renderList); renderList.append(d->seekHead.get()); + d->seekHead->addSizeListeners(renderList); + d->seekHead->addSizeListener(d->segment.get()); } d->segment->addSizeListeners(renderList); renderList.append(d->segment.get()); - // Render the elements - for(auto element : renderList) { - if(!element->render()) - return false; + // Render the elements. + // Because size changes of elements can cause segment offset updates and + // size changes in other elements, we might need multiple rounds until no more + // element needs rendering. + int renderRound = 0; + bool rendering = true; + while(rendering && renderRound < 5) { + rendering = false; + for(auto element : renderList) { + if(element->needsRender()) { + rendering = true; + if(!element->render()) { + return false; + } + } + } + ++renderRound; } // Write out to file diff --git a/taglib/matroska/matroskafile.h b/taglib/matroska/matroskafile.h index 7ac23fa3..66c1c07f 100644 --- a/taglib/matroska/matroskafile.h +++ b/taglib/matroska/matroskafile.h @@ -41,7 +41,8 @@ namespace TagLib::Matroska { * Constructs a Matroska file from \a file. If \a readProperties is \c true the * file's audio properties will also be read. * - * The \a readStyle parameter is currently unused. + * If \a readStyle is \c Accurate all seek head and cues segment positions + * are verified for the isValid() state of the file. */ explicit File(FileName file, bool readProperties = true, Properties::ReadStyle readStyle = Properties::Average); @@ -50,7 +51,8 @@ namespace TagLib::Matroska { * Constructs a Matroska file from \a stream. If \a readProperties is \c true the * file's audio properties will also be read. * - * The \a readStyle parameter is currently unused. + * If \a readStyle is \c Accurate all seek head and cues segment positions + * are verified for the isValid() state of the file. */ explicit File(IOStream *stream, bool readProperties = true, Properties::ReadStyle readStyle = Properties::Average); diff --git a/taglib/matroska/matroskaseekhead.cpp b/taglib/matroska/matroskaseekhead.cpp index 09a3070d..f297335e 100644 --- a/taglib/matroska/matroskaseekhead.cpp +++ b/taglib/matroska/matroskaseekhead.cpp @@ -27,27 +27,42 @@ using namespace TagLib; -Matroska::SeekHead::SeekHead() : - Element(static_cast(EBML::Element::Id::MkSeekHead)) +Matroska::SeekHead::SeekHead(offset_t segmentDataOffset) : + Element(static_cast(EBML::Element::Id::MkSeekHead)), + segmentDataOffset(segmentDataOffset) { + setNeedsRender(false); +} + +bool Matroska::SeekHead::isValid(TagLib::File& file) const +{ + bool result = true; + for(const auto &[id, offset] : entries) { + file.seek(segmentDataOffset + offset); + if(EBML::Element::readId(file) != id) { + debug(Utils::formatString("No ID %x found at seek position", id)); + result = false; + } + } + return result; } void Matroska::SeekHead::addEntry(const Element &element) { entries.append({element.id(), element.offset()}); debug("adding to seekhead"); - needsRender = true; + setNeedsRender(true); } void Matroska::SeekHead::addEntry(ID id, offset_t offset) { entries.append({id, offset}); - needsRender = true; + setNeedsRender(true); } ByteVector Matroska::SeekHead::renderInternal() { - auto beforeSize = size(); + auto beforeSize = sizeRenderedOrWritten(); EBML::MkSeekHead seekHead; seekHead.setMinRenderSize(beforeSize); for(const auto &[id, position] : entries) { @@ -65,26 +80,6 @@ ByteVector Matroska::SeekHead::renderInternal() return seekHead.render(); } -bool Matroska::SeekHead::render() -{ - if(!needsRender) - return true; - - auto beforeSize = size(); - auto data = renderInternal(); - needsRender = false; - auto afterSize = data.size(); - if(afterSize != beforeSize) { - return false; - // TODO handle expansion of seek head - if(!emitSizeChanged(afterSize - beforeSize)) - return false; - } - - setData(data); - return true; -} - void Matroska::SeekHead::write(File &file) { if(!data().isEmpty()) @@ -104,19 +99,33 @@ bool Matroska::SeekHead::sizeChanged(Element &caller, offset_t delta) return true; } else { - offset_t offset = caller.offset(); + // The equal case is needed when multiple new elements are added + // (e.g. Attachments and Tags), they will start with the same offset + // and are updated via size change handling. + offset_t offset = caller.offset() - segmentDataOffset; auto it = entries.begin(); while(it != entries.end()) { it = std::find_if(it, entries.end(), - [offset](const auto a){ return a.second > offset; } + [offset, callerID](const auto &a) { + return a.second >= offset && a.first != callerID; + } ); if(it != entries.end()) { it->second += delta; - needsRender = true; + setNeedsRender(true); ++it; } } + + if(caller.data().isEmpty() && caller.size() + delta == 0) { + // The caller element is removed, remove it from the seek head. + it = std::find_if(entries.begin(), entries.end(), + [callerID](const auto &a){ return a.first == callerID; }); + if(it != entries.end()) { + entries.erase(it); + } + } return true; } } diff --git a/taglib/matroska/matroskaseekhead.h b/taglib/matroska/matroskaseekhead.h index 4f4b09da..f41979a4 100644 --- a/taglib/matroska/matroskaseekhead.h +++ b/taglib/matroska/matroskaseekhead.h @@ -32,20 +32,20 @@ namespace TagLib { class SeekHead : public Element { public: - SeekHead(); + explicit SeekHead(offset_t segmentDataOffset); ~SeekHead() override = default; + bool isValid(TagLib::File &file) const; void addEntry(const Element &element); void addEntry(ID id, offset_t offset); - bool render() override; void write(TagLib::File &file) override; void sort(); //bool offsetChanged(Element &caller, offset_t delta) override; bool sizeChanged(Element &caller, offset_t delta) override; private: - ByteVector renderInternal(); + ByteVector renderInternal() override; List> entries; - bool needsRender = false; + const offset_t segmentDataOffset; }; } } diff --git a/taglib/matroska/matroskasegment.cpp b/taglib/matroska/matroskasegment.cpp index ddcffa90..19c32800 100644 --- a/taglib/matroska/matroskasegment.cpp +++ b/taglib/matroska/matroskasegment.cpp @@ -31,17 +31,27 @@ Matroska::Segment::Segment(offset_t sizeLength, offset_t dataSize, offset_t leng setSize(sizeLength); } +ByteVector Matroska::Segment::renderInternal() +{ + return EBML::renderVINT(dataSize, static_cast(sizeLength)); +} + bool Matroska::Segment::render() { - auto data = EBML::renderVINT(dataSize, static_cast(sizeLength)); - if(data.size() != sizeLength) { + auto beforeSize = sizeLength; + auto data = renderInternal(); + setNeedsRender(false); + auto afterSize = data.size(); + if(afterSize != beforeSize) { sizeLength = 8; - if(!emitSizeChanged(sizeLength - data.size())) - return false; - data = EBML::renderVINT(dataSize, static_cast(sizeLength)); - if(data.size() != sizeLength) + data = renderInternal(); + setNeedsRender(false); + afterSize = data.size(); + if(!emitSizeChanged(afterSize - beforeSize)) { return false; + } } + setData(data); return true; } @@ -49,5 +59,6 @@ bool Matroska::Segment::render() bool Matroska::Segment::sizeChanged(Element &, offset_t delta) { dataSize += delta; + setNeedsRender(true); return true; } diff --git a/taglib/matroska/matroskasegment.h b/taglib/matroska/matroskasegment.h index 0c9b2e0a..6e6d88bf 100644 --- a/taglib/matroska/matroskasegment.h +++ b/taglib/matroska/matroskasegment.h @@ -35,6 +35,8 @@ namespace TagLib::Matroska { offset_t dataOffset() const { return offset() + sizeLength; } private: + ByteVector renderInternal() override; + offset_t sizeLength; offset_t dataSize; }; diff --git a/taglib/matroska/matroskatag.cpp b/taglib/matroska/matroskatag.cpp index 082c5775..b4e7b215 100644 --- a/taglib/matroska/matroskatag.cpp +++ b/taglib/matroska/matroskatag.cpp @@ -87,6 +87,7 @@ Matroska::Tag::~Tag() = default; void Matroska::Tag::addSimpleTag(const SimpleTag &tag) { d->tags.append(tag); + setNeedsRender(true); } void Matroska::Tag::removeSimpleTag(const String &name, @@ -99,12 +100,14 @@ void Matroska::Tag::removeSimpleTag(const String &name, ); if(it != d->tags.end()) { d->tags.erase(it); + setNeedsRender(true); } } void Matroska::Tag::clearSimpleTags() { d->tags.clear(); + setNeedsRender(true); } const Matroska::SimpleTagsList &Matroska::Tag::simpleTagsList() const @@ -139,12 +142,12 @@ void Matroska::Tag::setGenre(const String &s) void Matroska::Tag::setYear(unsigned int i) { - d->setTag("DATE", String::number(i)); + d->setTag("DATE", i != 0 ? String::number(i) : String()); } void Matroska::Tag::setTrack(unsigned int i) { - d->setTag("TRACKNUMBER", String::number(i)); + d->setTag("TRACKNUMBER", i != 0 ? String::number(i) : String()); } String Matroska::Tag::title() const @@ -195,8 +198,13 @@ bool Matroska::Tag::isEmpty() const return d->tags.isEmpty(); } -bool Matroska::Tag::render() +ByteVector Matroska::Tag::renderInternal() { + if(d->tags.isEmpty()) { + // Avoid writing a Tags element without Tag element. + return {}; + } + EBML::MkTags tags; List targetList; @@ -266,16 +274,7 @@ bool Matroska::Tag::render() } tags.appendElement(std::move(tag)); } - - auto data = tags.render(); - auto beforeSize = size(); - auto afterSize = data.size(); - if(afterSize != beforeSize) { - if(!emitSizeChanged(afterSize - beforeSize)) - return false; - } - setData(data); - return true; + return tags.render(); } namespace @@ -421,6 +420,7 @@ PropertyMap Matroska::Tag::setProperties(const PropertyMap &propertyMap) if(it->type() == SimpleTag::StringType && !(key = translateTag(it->name(), it->targetTypeValue())).isEmpty()) { it = d->tags.erase(it); + setNeedsRender(true); } else { ++it; @@ -434,6 +434,7 @@ PropertyMap Matroska::Tag::setProperties(const PropertyMap &propertyMap) if(auto [name, targetTypeValue, _] = translateKey(key); !name.isEmpty()) { d->tags.append(SimpleTag(name, value, targetTypeValue)); + setNeedsRender(true); } else { unsupportedProperties[key] = values; @@ -529,6 +530,7 @@ bool Matroska::Tag::setComplexProperties(const String& key, const List(), targetTypeValue, language, defaultLanguage)); + setNeedsRender(true); result = true; } } diff --git a/taglib/matroska/matroskatag.h b/taglib/matroska/matroskatag.h index 8be38424..e9106b82 100644 --- a/taglib/matroska/matroskatag.h +++ b/taglib/matroska/matroskatag.h @@ -99,7 +99,7 @@ namespace TagLib { class TagPrivate; // private Element implementation - bool render() override; + ByteVector renderInternal() override; TAGLIB_MSVC_SUPPRESS_WARNING_NEEDS_TO_HAVE_DLL_INTERFACE std::unique_ptr d;