From b0bda317b720775ec66cc771e26a536673599968 Mon Sep 17 00:00:00 2001 From: Nathan Froyd Date: Tue, 20 Nov 2018 10:10:31 -0500 Subject: [PATCH] Bug 1507922 - make ownership of MetadataTags more clear in the ogg code; r=gerald,jya Use UniquePtr for return types, so it's obvious who has ownership. --- dom/media/MediaDecoder.cpp | 2 +- dom/media/MediaMetadataManager.h | 11 ++++++----- dom/media/flac/FlacDemuxer.cpp | 4 ++-- dom/media/flac/FlacFrameParser.cpp | 6 ++---- dom/media/flac/FlacFrameParser.h | 2 +- dom/media/ogg/OggCodecState.cpp | 18 +++++++++--------- dom/media/ogg/OggCodecState.h | 13 +++++++------ dom/media/ogg/OggDemuxer.cpp | 8 ++++---- dom/media/ogg/OggDemuxer.h | 2 +- 9 files changed, 33 insertions(+), 33 deletions(-) diff --git a/dom/media/MediaDecoder.cpp b/dom/media/MediaDecoder.cpp index fbd21fc8017c..7feb93b07710 100644 --- a/dom/media/MediaDecoder.cpp +++ b/dom/media/MediaDecoder.cpp @@ -595,7 +595,7 @@ void MediaDecoder::OnMetadataUpdate(TimedMetadata&& aMetadata) { AbstractThread::AutoEnter context(AbstractMainThread()); GetOwner()->RemoveMediaTracks(); MetadataLoaded(MakeUnique(*aMetadata.mInfo), - UniquePtr(aMetadata.mTags.forget()), + UniquePtr(std::move(aMetadata.mTags)), MediaDecoderEventVisibility::Observable); FirstFrameLoaded(std::move(aMetadata.mInfo), MediaDecoderEventVisibility::Observable); diff --git a/dom/media/MediaMetadataManager.h b/dom/media/MediaMetadataManager.h index 494627d7136a..3ebd129ed42c 100644 --- a/dom/media/MediaMetadataManager.h +++ b/dom/media/MediaMetadataManager.h @@ -26,10 +26,11 @@ typedef MediaEventSourceExc TimedMetadataEventSource; class TimedMetadata : public LinkedListElement { public: TimedMetadata(const media::TimeUnit& aPublishTime, - nsAutoPtr&& aTags, nsAutoPtr&& aInfo) - : mPublishTime(aPublishTime), - mTags(std::move(aTags)), - mInfo(std::move(aInfo)) {} + UniquePtr&& aTags, + nsAutoPtr&& aInfo) + : mPublishTime(aPublishTime) + , mTags(std::move(aTags)) + , mInfo(std::move(aInfo)) {} // Define our move constructor because we don't want to move the members of // LinkedListElement to change the list. @@ -42,7 +43,7 @@ class TimedMetadata : public LinkedListElement { media::TimeUnit mPublishTime; // The metadata. The ownership is transfered to the element when dispatching // to the main threads. - nsAutoPtr mTags; + UniquePtr mTags; // The media info, including the info of audio tracks and video tracks. // The ownership is transfered to MediaDecoder when dispatching to the // main thread. diff --git a/dom/media/flac/FlacDemuxer.cpp b/dom/media/flac/FlacDemuxer.cpp index eabecddf35d1..7488220ddf41 100644 --- a/dom/media/flac/FlacDemuxer.cpp +++ b/dom/media/flac/FlacDemuxer.cpp @@ -427,7 +427,7 @@ class FrameParser { AudioInfo Info() const { return mParser.mInfo; } // Return a hash table with tag metadata. - MetadataTags* GetTags() const { return mParser.GetTags(); } + UniquePtr GetTags() const { return mParser.GetTags(); } private: bool GetNextFrame(MediaResourceIndex& aResource) { @@ -644,7 +644,7 @@ UniquePtr FlacTrackDemuxer::GetInfo() const { if (mParser->Info().IsValid()) { // We have a proper metadata header. UniquePtr info = mParser->Info().Clone(); - nsAutoPtr tags(mParser->GetTags()); + UniquePtr tags(mParser->GetTags()); if (tags) { for (auto iter = tags->Iter(); !iter.Done(); iter.Next()) { info->mTags.AppendElement(MetadataTag(iter.Key(), iter.Data())); diff --git a/dom/media/flac/FlacFrameParser.cpp b/dom/media/flac/FlacFrameParser.cpp index 8ed889cd4864..18e341a11604 100644 --- a/dom/media/flac/FlacFrameParser.cpp +++ b/dom/media/flac/FlacFrameParser.cpp @@ -223,14 +223,12 @@ Result FlacFrameParser::IsHeaderBlock(const uint8_t* aPacket, return type >= 1 && type <= 6; } -MetadataTags* FlacFrameParser::GetTags() const { +UniquePtr FlacFrameParser::GetTags() const { if (!mParser) { return nullptr; } - MetadataTags* tags; - - tags = new MetadataTags; + auto tags = MakeUnique(); for (uint32_t i = 0; i < mParser->mTags.Length(); i++) { OggCodecState::AddVorbisComment(tags, mParser->mTags[i].Data(), mParser->mTags[i].Length()); diff --git a/dom/media/flac/FlacFrameParser.h b/dom/media/flac/FlacFrameParser.h index 70f2b1317d8a..57e4ebab4e0e 100644 --- a/dom/media/flac/FlacFrameParser.h +++ b/dom/media/flac/FlacFrameParser.h @@ -49,7 +49,7 @@ class FlacFrameParser { int64_t BlockDuration(const uint8_t* aPacket, size_t aLength) const; // Return a hash table with tag metadata. - MetadataTags* GetTags() const; + UniquePtr GetTags() const; AudioInfo mInfo; diff --git a/dom/media/ogg/OggCodecState.cpp b/dom/media/ogg/OggCodecState.cpp index 7c0fd5b63d1f..08f9881dfd5e 100644 --- a/dom/media/ogg/OggCodecState.cpp +++ b/dom/media/ogg/OggCodecState.cpp @@ -95,7 +95,8 @@ bool OggCodecState::IsValidVorbisTagName(nsCString& aName) { return true; } -bool OggCodecState::AddVorbisComment(MetadataTags* aTags, const char* aComment, +bool OggCodecState::AddVorbisComment(UniquePtr& aTags, + const char* aComment, uint32_t aLength) { const char* div = (const char*)memchr(aComment, '=', aLength); if (!div) { @@ -712,11 +713,10 @@ bool VorbisState::IsHeader(ogg_packet* aPacket) { return aPacket->bytes > 0 ? (aPacket->packet[0] & 0x1) : false; } -MetadataTags* VorbisState::GetTags() { - MetadataTags* tags; +UniquePtr VorbisState::GetTags() { NS_ASSERTION(mComment.user_comments, "no vorbis comment strings!"); NS_ASSERTION(mComment.comment_lengths, "no vorbis comment lengths!"); - tags = new MetadataTags; + auto tags = MakeUnique(); for (int i = 0; i < mComment.comments; i++) { AddVorbisComment(tags, mComment.user_comments[i], mComment.comment_lengths[i]); @@ -967,10 +967,8 @@ bool OpusState::DecodeHeader(OggPacketPtr aPacket) { } /* Construct and return a tags hashmap from our internal array */ -MetadataTags* OpusState::GetTags() { - MetadataTags* tags; - - tags = new MetadataTags; +UniquePtr OpusState::GetTags() { + auto tags = MakeUnique(); for (uint32_t i = 0; i < mParser->mTags.Length(); i++) { AddVorbisComment(tags, mParser->mTags[i].Data(), mParser->mTags[i].Length()); @@ -1228,7 +1226,9 @@ nsresult FlacState::PageIn(ogg_page* aPage) { } // Return a hash table with tag metadata. -MetadataTags* FlacState::GetTags() { return mParser.GetTags(); } +UniquePtr FlacState::GetTags() { + return mParser.GetTags(); +} const TrackInfo* FlacState::GetInfo() const { return &mParser.mInfo; } diff --git a/dom/media/ogg/OggCodecState.h b/dom/media/ogg/OggCodecState.h index 2a26fbae7160..bc7d66a0ad05 100644 --- a/dom/media/ogg/OggCodecState.h +++ b/dom/media/ogg/OggCodecState.h @@ -118,7 +118,7 @@ class OggCodecState { } // Build a hash table with tag metadata parsed from the stream. - virtual MetadataTags* GetTags() { return nullptr; } + virtual UniquePtr GetTags() { return nullptr; } // Returns the end time that a granulepos represents. virtual int64_t Time(int64_t granulepos) { return -1; } @@ -241,10 +241,11 @@ class OggCodecState { // Utility method to parse and add a vorbis-style comment // to a metadata hash table. Most Ogg-encapsulated codecs // use the vorbis comment format for metadata. - static bool AddVorbisComment(MetadataTags* aTags, const char* aComment, + static bool AddVorbisComment(UniquePtr& aTags, + const char* aComment, uint32_t aLength); - protected: +protected: // Constructs a new OggCodecState. aActive denotes whether the stream is // active. For streams of unsupported or unknown types, aActive should be // false. @@ -289,7 +290,7 @@ class VorbisState : public OggCodecState { const TrackInfo* GetInfo() const override { return &mInfo; } // Return a hash table with tag metadata. - MetadataTags* GetTags() override; + UniquePtr GetTags() override; private: AudioInfo mInfo; @@ -408,7 +409,7 @@ class OpusState : public OggCodecState { static int64_t Time(int aPreSkip, int64_t aGranulepos); // Construct and return a table of tags from the metadata header. - MetadataTags* GetTags() override; + UniquePtr GetTags() override; private: nsAutoPtr mParser; @@ -582,7 +583,7 @@ class FlacState : public OggCodecState { nsresult PageIn(ogg_page* aPage) override; // Return a hash table with tag metadata. - MetadataTags* GetTags() override; + UniquePtr GetTags() override; const TrackInfo* GetInfo() const override; diff --git a/dom/media/ogg/OggDemuxer.cpp b/dom/media/ogg/OggDemuxer.cpp index 87580f830411..0beef3bbc2da 100644 --- a/dom/media/ogg/OggDemuxer.cpp +++ b/dom/media/ogg/OggDemuxer.cpp @@ -392,12 +392,12 @@ void OggDemuxer::SetupMediaTracksInfo(const nsTArray& aSerials) { } } -void OggDemuxer::FillTags(TrackInfo* aInfo, MetadataTags* aTags) { +void OggDemuxer::FillTags(TrackInfo* aInfo, UniquePtr&& aTags) { if (!aTags) { return; } - nsAutoPtr tags(aTags); - for (auto iter = aTags->Iter(); !iter.Done(); iter.Next()) { + UniquePtr tags(std::move(aTags)); + for (auto iter = tags->Iter(); !iter.Done(); iter.Next()) { aInfo->mTags.AppendElement(MetadataTag(iter.Key(), iter.Data())); } } @@ -567,7 +567,7 @@ bool OggDemuxer::ReadOggChain(const media::TimeUnit& aLastEndTime) { OpusState* newOpusState = nullptr; VorbisState* newVorbisState = nullptr; FlacState* newFlacState = nullptr; - nsAutoPtr tags; + UniquePtr tags; if (HasVideo() || HasSkeleton() || !HasAudio()) { return false; diff --git a/dom/media/ogg/OggDemuxer.h b/dom/media/ogg/OggDemuxer.h index b3efca811ec6..9d7dbc889496 100644 --- a/dom/media/ogg/OggDemuxer.h +++ b/dom/media/ogg/OggDemuxer.h @@ -194,7 +194,7 @@ class OggDemuxer : public MediaDataDemuxer, void SetupTarget(OggCodecState** aSavedState, OggCodecState* aNewState); void SetupTargetSkeleton(); void SetupMediaTracksInfo(const nsTArray& aSerials); - void FillTags(TrackInfo* aInfo, MetadataTags* aTags); + void FillTags(TrackInfo* aInfo, UniquePtr&& aTags); // Compute an ogg page's checksum ogg_uint32_t GetPageChecksum(ogg_page* aPage);