Bug 1507922 - make ownership of MetadataTags more clear in the ogg code; r=gerald,jya
authorNathan Froyd <froydnj@mozilla.com>
Tue, 20 Nov 2018 10:10:31 -0500
changeset 503704 ce4515855b43514b4ee6e5ef8d450ed2dbfb1a73
parent 503703 2584f7a7f63adb79ce5acd99bbb91f6b64b87f0f
child 503705 78d26b6dce21a0bd8ce11a624a3bd26779d44937
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgerald, jya
bugs1507922
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
dom/media/MediaMetadataManager.h
dom/media/flac/FlacDemuxer.cpp
dom/media/flac/FlacFrameParser.cpp
dom/media/flac/FlacFrameParser.h
dom/media/ogg/OggCodecState.cpp
dom/media/ogg/OggCodecState.h
dom/media/ogg/OggDemuxer.cpp
dom/media/ogg/OggDemuxer.h
--- a/dom/media/MediaDecoder.cpp
+++ b/dom/media/MediaDecoder.cpp
@@ -590,17 +590,17 @@ double MediaDecoder::GetCurrentTime() {
   return mLogicalPosition;
 }
 
 void MediaDecoder::OnMetadataUpdate(TimedMetadata&& aMetadata) {
   MOZ_ASSERT(NS_IsMainThread());
   AbstractThread::AutoEnter context(AbstractMainThread());
   GetOwner()->RemoveMediaTracks();
   MetadataLoaded(MakeUnique<MediaInfo>(*aMetadata.mInfo),
-                 UniquePtr<MetadataTags>(aMetadata.mTags.forget()),
+                 UniquePtr<MetadataTags>(std::move(aMetadata.mTags)),
                  MediaDecoderEventVisibility::Observable);
   FirstFrameLoaded(std::move(aMetadata.mInfo),
                    MediaDecoderEventVisibility::Observable);
 }
 
 void MediaDecoder::MetadataLoaded(
     UniquePtr<MediaInfo> aInfo, UniquePtr<MetadataTags> aTags,
     MediaDecoderEventVisibility aEventVisibility) {
--- a/dom/media/MediaMetadataManager.h
+++ b/dom/media/MediaMetadataManager.h
@@ -21,33 +21,34 @@ class TimedMetadata;
 typedef MediaEventProducerExc<TimedMetadata> TimedMetadataEventProducer;
 typedef MediaEventSourceExc<TimedMetadata> TimedMetadataEventSource;
 
 // A struct that contains the metadata of a media, and the time at which those
 // metadata should start to be reported.
 class TimedMetadata : public LinkedListElement<TimedMetadata> {
  public:
   TimedMetadata(const media::TimeUnit& aPublishTime,
-                nsAutoPtr<MetadataTags>&& aTags, nsAutoPtr<MediaInfo>&& aInfo)
-      : mPublishTime(aPublishTime),
-        mTags(std::move(aTags)),
-        mInfo(std::move(aInfo)) {}
+                UniquePtr<MetadataTags>&& aTags,
+                nsAutoPtr<MediaInfo>&& 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.
   TimedMetadata(TimedMetadata&& aOther)
       : mPublishTime(aOther.mPublishTime),
         mTags(std::move(aOther.mTags)),
         mInfo(std::move(aOther.mInfo)) {}
 
   // The time, in microseconds, at which those metadata should be available.
   media::TimeUnit mPublishTime;
   // The metadata. The ownership is transfered to the element when dispatching
   // to the main threads.
-  nsAutoPtr<MetadataTags> mTags;
+  UniquePtr<MetadataTags> 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.
   nsAutoPtr<MediaInfo> mInfo;
 };
 
 // This class encapsulate the logic to give the metadata from the reader to
 // the content, at the right time.
--- a/dom/media/flac/FlacDemuxer.cpp
+++ b/dom/media/flac/FlacDemuxer.cpp
@@ -422,17 +422,17 @@ class FrameParser {
     return mParser.DecodeHeaderBlock(aPacket, aLength).isOk();
   }
 
   bool HasFullMetadata() const { return mParser.HasFullMetadata(); }
 
   AudioInfo Info() const { return mParser.mInfo; }
 
   // Return a hash table with tag metadata.
-  MetadataTags* GetTags() const { return mParser.GetTags(); }
+  UniquePtr<MetadataTags> GetTags() const { return mParser.GetTags(); }
 
  private:
   bool GetNextFrame(MediaResourceIndex& aResource) {
     while (mNextFrame.FindNext(aResource)) {
       // Move our offset slightly, so that we don't find the same frame at the
       // next FindNext call.
       aResource.Seek(SEEK_CUR, mNextFrame.Header().Size());
       if (mFrame.IsValid() &&
@@ -639,17 +639,17 @@ bool FlacTrackDemuxer::Init() {
 
   return true;
 }
 
 UniquePtr<TrackInfo> FlacTrackDemuxer::GetInfo() const {
   if (mParser->Info().IsValid()) {
     // We have a proper metadata header.
     UniquePtr<TrackInfo> info = mParser->Info().Clone();
-    nsAutoPtr<MetadataTags> tags(mParser->GetTags());
+    UniquePtr<MetadataTags> tags(mParser->GetTags());
     if (tags) {
       for (auto iter = tags->Iter(); !iter.Done(); iter.Next()) {
         info->mTags.AppendElement(MetadataTag(iter.Key(), iter.Data()));
       }
     }
     return info;
   } else if (mParser->FirstFrame().Info().IsValid()) {
     // Use the first frame header.
--- a/dom/media/flac/FlacFrameParser.cpp
+++ b/dom/media/flac/FlacFrameParser.cpp
@@ -218,24 +218,22 @@ Result<bool, nsresult> FlacFrameParser::
     MOZ_TRY_VAR(blockType, br.ReadU8());
     blockType &= 0x7f;
     return blockType == FLAC_METADATA_TYPE_STREAMINFO;
   }
   char type = aPacket[0] & 0x7f;
   return type >= 1 && type <= 6;
 }
 
-MetadataTags* FlacFrameParser::GetTags() const {
+UniquePtr<MetadataTags> FlacFrameParser::GetTags() const {
   if (!mParser) {
     return nullptr;
   }
 
-  MetadataTags* tags;
-
-  tags = new MetadataTags;
+  auto tags = MakeUnique<MetadataTags>();
   for (uint32_t i = 0; i < mParser->mTags.Length(); i++) {
     OggCodecState::AddVorbisComment(tags, mParser->mTags[i].Data(),
                                     mParser->mTags[i].Length());
   }
 
   return tags;
 }
 
--- a/dom/media/flac/FlacFrameParser.h
+++ b/dom/media/flac/FlacFrameParser.h
@@ -44,17 +44,17 @@ class FlacFrameParser {
   Result<Ok, nsresult> DecodeHeaderBlock(const uint8_t* aPacket,
                                          size_t aLength);
   bool HasFullMetadata() const { return mFullMetadata; }
   // Return the duration in frames found in the block. -1 if error
   // such as invalid packet.
   int64_t BlockDuration(const uint8_t* aPacket, size_t aLength) const;
 
   // Return a hash table with tag metadata.
-  MetadataTags* GetTags() const;
+  UniquePtr<MetadataTags> GetTags() const;
 
   AudioInfo mInfo;
 
  private:
   bool ReconstructFlacGranulepos(void);
   Maybe<uint32_t> mNumHeaders;
   uint32_t mMinBlockSize;
   uint32_t mMaxBlockSize;
--- a/dom/media/ogg/OggCodecState.cpp
+++ b/dom/media/ogg/OggCodecState.cpp
@@ -90,17 +90,18 @@ bool OggCodecState::IsValidVorbisTagName
   for (uint32_t i = 0; i < length; i++) {
     if (data[i] < 0x20 || data[i] > 0x7D || data[i] == '=') {
       return false;
     }
   }
   return true;
 }
 
-bool OggCodecState::AddVorbisComment(MetadataTags* aTags, const char* aComment,
+bool OggCodecState::AddVorbisComment(UniquePtr<MetadataTags>& aTags,
+                                     const char* aComment,
                                      uint32_t aLength) {
   const char* div = (const char*)memchr(aComment, '=', aLength);
   if (!div) {
     LOG(LogLevel::Debug, ("Skipping comment: no separator"));
     return false;
   }
   nsCString key = nsCString(aComment, div - aComment);
   if (!IsValidVorbisTagName(key)) {
@@ -707,21 +708,20 @@ bool VorbisState::IsHeader(ogg_packet* a
   // The first byte in each Vorbis header packet is either 0x01, 0x03, or 0x05,
   // i.e. the first bit is odd. Audio data packets have their first bit as 0x0.
   // Any packet with its first bit set cannot be a data packet, it's a
   // (possibly invalid) header packet.
   // See: http://xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-610004.2.1
   return aPacket->bytes > 0 ? (aPacket->packet[0] & 0x1) : false;
 }
 
-MetadataTags* VorbisState::GetTags() {
-  MetadataTags* tags;
+UniquePtr<MetadataTags> 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<MetadataTags>();
   for (int i = 0; i < mComment.comments; i++) {
     AddVorbisComment(tags, mComment.user_comments[i],
                      mComment.comment_lengths[i]);
   }
   return tags;
 }
 
 nsresult VorbisState::PageIn(ogg_page* aPage) {
@@ -962,20 +962,18 @@ bool OpusState::DecodeHeader(OggPacketPt
       // Put it back on the queue so we can decode it.
       mPackets.PushFront(std::move(aPacket));
       break;
   }
   return true;
 }
 
 /* Construct and return a tags hashmap from our internal array */
-MetadataTags* OpusState::GetTags() {
-  MetadataTags* tags;
-
-  tags = new MetadataTags;
+UniquePtr<MetadataTags> OpusState::GetTags() {
+  auto tags = MakeUnique<MetadataTags>();
   for (uint32_t i = 0; i < mParser->mTags.Length(); i++) {
     AddVorbisComment(tags, mParser->mTags[i].Data(),
                      mParser->mTags[i].Length());
   }
 
   return tags;
 }
 
@@ -1223,17 +1221,19 @@ nsresult FlacState::PageIn(ogg_page* aPa
       mPackets.Append(std::move(packet));
     }
     mUnstamped.Clear();
   }
   return NS_OK;
 }
 
 // Return a hash table with tag metadata.
-MetadataTags* FlacState::GetTags() { return mParser.GetTags(); }
+UniquePtr<MetadataTags> FlacState::GetTags() {
+  return mParser.GetTags();
+}
 
 const TrackInfo* FlacState::GetInfo() const { return &mParser.mInfo; }
 
 bool FlacState::ReconstructFlacGranulepos(void) {
   NS_ASSERTION(mUnstamped.Length() > 0, "Must have unstamped packets");
   auto& last = mUnstamped.LastElement();
   NS_ASSERTION(last->e_o_s || last->granulepos > 0,
                "Must know last granulepos!");
--- a/dom/media/ogg/OggCodecState.h
+++ b/dom/media/ogg/OggCodecState.h
@@ -113,17 +113,17 @@ class OggCodecState {
   // to determine if the last header has been read.
   // This function takes ownership of the packet and is responsible for
   // releasing it or queuing it for later processing.
   virtual bool DecodeHeader(OggPacketPtr aPacket) {
     return (mDoneReadingHeaders = true);
   }
 
   // Build a hash table with tag metadata parsed from the stream.
-  virtual MetadataTags* GetTags() { return nullptr; }
+  virtual UniquePtr<MetadataTags> GetTags() { return nullptr; }
 
   // Returns the end time that a granulepos represents.
   virtual int64_t Time(int64_t granulepos) { return -1; }
 
   // Returns the start time that a granulepos represents.
   virtual int64_t StartTime(int64_t granulepos) { return -1; }
 
   // Returns the duration of the given packet, if it can be determined.
@@ -236,20 +236,21 @@ class OggCodecState {
   }
 
   // Validation utility for vorbis-style tag names.
   static bool IsValidVorbisTagName(nsCString& aName);
 
   // 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<MetadataTags>& 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.
   OggCodecState(ogg_page* aBosPage, bool aActive);
 
   // Deallocates all packets stored in mUnstamped, and clears the array.
   void ClearUnstamped();
 
@@ -284,17 +285,17 @@ class VorbisState : public OggCodecState
   int64_t PacketDuration(ogg_packet* aPacket) override;
   bool Init() override;
   nsresult Reset() override;
   bool IsHeader(ogg_packet* aPacket) override;
   nsresult PageIn(ogg_page* aPage) override;
   const TrackInfo* GetInfo() const override { return &mInfo; }
 
   // Return a hash table with tag metadata.
-  MetadataTags* GetTags() override;
+  UniquePtr<MetadataTags> GetTags() override;
 
  private:
   AudioInfo mInfo;
   vorbis_info mVorbisInfo;
   vorbis_comment mComment;
   vorbis_dsp_state mDsp;
   vorbis_block mBlock;
   OggPacketQueue mHeaders;
@@ -403,17 +404,17 @@ class OpusState : public OggCodecState {
   nsresult PageIn(ogg_page* aPage) override;
   already_AddRefed<MediaRawData> PacketOutAsMediaRawData() override;
   const TrackInfo* GetInfo() const override { return &mInfo; }
 
   // Returns the end time that a granulepos represents.
   static int64_t Time(int aPreSkip, int64_t aGranulepos);
 
   // Construct and return a table of tags from the metadata header.
-  MetadataTags* GetTags() override;
+  UniquePtr<MetadataTags> GetTags() override;
 
  private:
   nsAutoPtr<OpusParser> mParser;
   OpusMSDecoder* mDecoder;
 
   // Granule position (end sample) of the last decoded Opus packet. This is
   // used to calculate the amount we should trim from the last packet.
   int64_t mPrevPacketGranulepos;
@@ -577,17 +578,17 @@ class FlacState : public OggCodecState {
   CodecType GetType() override { return TYPE_FLAC; }
   bool DecodeHeader(OggPacketPtr aPacket) override;
   int64_t Time(int64_t granulepos) override;
   int64_t PacketDuration(ogg_packet* aPacket) override;
   bool IsHeader(ogg_packet* aPacket) override;
   nsresult PageIn(ogg_page* aPage) override;
 
   // Return a hash table with tag metadata.
-  MetadataTags* GetTags() override;
+  UniquePtr<MetadataTags> GetTags() override;
 
   const TrackInfo* GetInfo() const override;
 
  private:
   bool ReconstructFlacGranulepos(void);
 
   FlacFrameParser mParser;
 };
--- a/dom/media/ogg/OggDemuxer.cpp
+++ b/dom/media/ogg/OggDemuxer.cpp
@@ -387,22 +387,22 @@ void OggDemuxer::SetupMediaTracksInfo(co
             true);
       }
       FillTags(isAudio ? static_cast<TrackInfo*>(&mInfo.mAudio) : &mInfo.mVideo,
                primeState->GetTags());
     }
   }
 }
 
-void OggDemuxer::FillTags(TrackInfo* aInfo, MetadataTags* aTags) {
+void OggDemuxer::FillTags(TrackInfo* aInfo, UniquePtr<MetadataTags>&& aTags) {
   if (!aTags) {
     return;
   }
-  nsAutoPtr<MetadataTags> tags(aTags);
-  for (auto iter = aTags->Iter(); !iter.Done(); iter.Next()) {
+  UniquePtr<MetadataTags> tags(std::move(aTags));
+  for (auto iter = tags->Iter(); !iter.Done(); iter.Next()) {
     aInfo->mTags.AppendElement(MetadataTag(iter.Key(), iter.Data()));
   }
 }
 
 nsresult OggDemuxer::ReadMetadata() {
   OGG_DEBUG("OggDemuxer::ReadMetadata called!");
 
   // We read packets until all bitstreams have read all their header packets.
@@ -562,17 +562,17 @@ void OggDemuxer::SetChained() {
   }
 }
 
 bool OggDemuxer::ReadOggChain(const media::TimeUnit& aLastEndTime) {
   bool chained = false;
   OpusState* newOpusState = nullptr;
   VorbisState* newVorbisState = nullptr;
   FlacState* newFlacState = nullptr;
-  nsAutoPtr<MetadataTags> tags;
+  UniquePtr<MetadataTags> tags;
 
   if (HasVideo() || HasSkeleton() || !HasAudio()) {
     return false;
   }
 
   ogg_page page;
   if (!ReadOggPage(TrackInfo::kAudioTrack, &page) || !ogg_page_bos(&page)) {
     // Chaining is only supported for audio only ogg files.
--- a/dom/media/ogg/OggDemuxer.h
+++ b/dom/media/ogg/OggDemuxer.h
@@ -189,17 +189,17 @@ class OggDemuxer : public MediaDataDemux
   // Fills aTracks with the serial numbers of each active stream, for use by
   // various SkeletonState functions.
   void BuildSerialList(nsTArray<uint32_t>& aTracks);
 
   // Setup target bitstreams for decoding.
   void SetupTarget(OggCodecState** aSavedState, OggCodecState* aNewState);
   void SetupTargetSkeleton();
   void SetupMediaTracksInfo(const nsTArray<uint32_t>& aSerials);
-  void FillTags(TrackInfo* aInfo, MetadataTags* aTags);
+  void FillTags(TrackInfo* aInfo, UniquePtr<MetadataTags>&& aTags);
 
   // Compute an ogg page's checksum
   ogg_uint32_t GetPageChecksum(ogg_page* aPage);
 
   // Get the end time of aEndOffset. This is the playback position we'd reach
   // after playback finished at aEndOffset.
   int64_t RangeEndTime(TrackInfo::TrackType aType, int64_t aEndOffset);