Bug 1584721 - P2. Remove the need to explicitly inform child/parent that a Shmem is no longer in use. r=mjf
authorJean-Yves Avenard <jyavenard@mozilla.com>
Tue, 01 Oct 2019 11:23:25 +0000
changeset 495738 b4de320b347971aa9a0e3b7ba3519c72b656b6f8
parent 495737 ae74c8a033d0e52ddf1ce6d9e2122347c52e1c4b
child 495739 68be28f880f3fbb8874698e541165ffe02e6ffdb
push id96833
push userjyavenard@mozilla.com
push dateTue, 01 Oct 2019 16:30:16 +0000
treeherderautoland@3f6d096c5358 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmjf
bugs1584721
milestone71.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 1584721 - P2. Remove the need to explicitly inform child/parent that a Shmem is no longer in use. r=mjf We can deduct it from the workflow itself. Also fix potential Shmem leak. Differential Revision: https://phabricator.services.mozilla.com/D47261
dom/media/ipc/PRemoteDecoder.ipdl
dom/media/ipc/RemoteAudioDecoder.cpp
dom/media/ipc/RemoteAudioDecoder.h
dom/media/ipc/RemoteDecoderChild.cpp
dom/media/ipc/RemoteDecoderChild.h
dom/media/ipc/RemoteDecoderParent.cpp
dom/media/ipc/RemoteDecoderParent.h
--- a/dom/media/ipc/PRemoteDecoder.ipdl
+++ b/dom/media/ipc/PRemoteDecoder.ipdl
@@ -83,17 +83,12 @@ parent:
   // on the manager protocol.
   async Decode(MediaRawDataIPDL data) returns (DecodeResultIPDL result);
   async Flush() returns (MediaResult error);
   async Drain() returns (DecodeResultIPDL result);
   async Shutdown() returns (bool unused);
   // To clear the threshold, call with INT64_MIN.
   async SetSeekThreshold(TimeUnit time);
 
-  async DoneWithOutput(Shmem outputShmem);
-
   async __delete__();
-
-  child:
-  async DoneWithInput(Shmem inputShmem);
 };
 
 } // namespace mozilla
--- a/dom/media/ipc/RemoteAudioDecoder.cpp
+++ b/dom/media/ipc/RemoteAudioDecoder.cpp
@@ -32,18 +32,16 @@ MediaResult RemoteAudioDecoderChild::Pro
             std::min((unsigned long)data.audioDataBufferSize(),
                      (unsigned long)data.buffer().Size<AudioDataValue>()))) {
       // OOM
       return MediaResult(NS_ERROR_OUT_OF_MEMORY, __func__);
     }
     PodCopy(alignedAudioBuffer.Data(), data.buffer().get<AudioDataValue>(),
             alignedAudioBuffer.Length());
 
-    Unused << SendDoneWithOutput(std::move(data.buffer()));
-
     RefPtr<AudioData> audio = new AudioData(
         data.base().offset(), data.base().time(), std::move(alignedAudioBuffer),
         data.channels(), data.rate(), data.channelMap());
 
     mDecodedData.AppendElement(std::move(audio));
   }
   return NS_OK;
 }
@@ -77,17 +75,18 @@ MediaResult RemoteAudioDecoderChild::Ini
 }
 
 RemoteAudioDecoderParent::RemoteAudioDecoderParent(
     RemoteDecoderManagerParent* aParent, const AudioInfo& aAudioInfo,
     const CreateDecoderParams::OptionSet& aOptions,
     TaskQueue* aManagerTaskQueue, TaskQueue* aDecodeTaskQueue, bool* aSuccess,
     nsCString* aErrorDescription)
     : RemoteDecoderParent(aParent, aManagerTaskQueue, aDecodeTaskQueue),
-      mAudioInfo(aAudioInfo) {
+      mAudioInfo(aAudioInfo),
+      mDecodedFramePool(4) {
   CreateDecoderParams params(mAudioInfo);
   params.mTaskQueue = mDecodeTaskQueue;
   params.mOptions = aOptions;
   MediaResult error(NS_OK);
   params.mError = &error;
 
   if (VorbisDataDecoder::IsVorbis(params.mConfig.mMimeType)) {
     mDecoder = new VorbisDataDecoder(params);
@@ -105,49 +104,70 @@ RemoteAudioDecoderParent::RemoteAudioDec
   *aSuccess = !!mDecoder;
 }
 
 MediaResult RemoteAudioDecoderParent::ProcessDecodedData(
     const MediaDataDecoder::DecodedData& aData,
     DecodedOutputIPDL& aDecodedData) {
   MOZ_ASSERT(OnManagerThread());
 
+  // If we are here, we know all previously returned RemoteAudioDataIPDL got
+  // used by the child. We can mark all previously sent ShmemBuffer as available
+  // again.
+  ReleaseUsedShmems();
+
   nsTArray<RemoteAudioDataIPDL> array;
 
   for (const auto& data : aData) {
     MOZ_ASSERT(data->mType == MediaData::Type::AUDIO_DATA,
                "Can only decode audio using RemoteAudioDecoderParent!");
     AudioData* audio = static_cast<AudioData*>(data.get());
 
     MOZ_ASSERT(audio->Data().Elements(),
                "Decoded audio must output an AlignedAudioBuffer "
                "to be used with RemoteAudioDecoderParent");
 
     ShmemBuffer buffer = mDecodedFramePool.Get(
-        this, audio->Data().Length() * sizeof(AudioDataValue));
+        this, audio->Data().Length() * sizeof(AudioDataValue),
+        ShmemPool::AllocationPolicy::Unsafe);
     if (!buffer.Valid()) {
       return MediaResult(NS_ERROR_OUT_OF_MEMORY,
                          "ShmemBuffer::Get failed in "
                          "RemoteAudioDecoderParent::ProcessDecodedData");
     }
     if (audio->Data().Length() > buffer.Get().Size<AudioDataValue>()) {
+      mDecodedFramePool.Put(std::move(buffer));
       return MediaResult(NS_ERROR_OUT_OF_MEMORY,
                          "ShmemBuffer::Get returned less than requested in "
                          "RemoteAudioDecoderParent::ProcessDecodedData");
     }
 
     PodCopy(buffer.Get().get<AudioDataValue>(), audio->Data().Elements(),
             audio->Data().Length());
 
+    mUsedShmems.AppendElement(buffer.Get());
+
     RemoteAudioDataIPDL output(
         MediaDataIPDL(data->mOffset, data->mTime, data->mTimecode,
                       data->mDuration, data->mKeyframe),
         audio->mChannels, audio->mRate, audio->mChannelMap,
         audio->Data().Length(), std::move(buffer.Get()));
     array.AppendElement(output);
   }
 
   aDecodedData = std::move(array);
 
   return NS_OK;
 }
 
+void RemoteAudioDecoderParent::CleanupOnActorDestroy() {
+  ReleaseUsedShmems();
+  mDecodedFramePool.Cleanup(this);
+}
+
+void RemoteAudioDecoderParent::ReleaseUsedShmems() {
+  for (ShmemBuffer& mem : mUsedShmems) {
+    mDecodedFramePool.Put(ShmemBuffer(mem.Get()));
+  }
+  mUsedShmems.Clear();
+}
+
 }  // namespace mozilla
--- a/dom/media/ipc/RemoteAudioDecoder.h
+++ b/dom/media/ipc/RemoteAudioDecoder.h
@@ -30,21 +30,25 @@ class RemoteAudioDecoderParent final : p
                            const CreateDecoderParams::OptionSet& aOptions,
                            TaskQueue* aManagerTaskQueue,
                            TaskQueue* aDecodeTaskQueue, bool* aSuccess,
                            nsCString* aErrorDescription);
 
  protected:
   MediaResult ProcessDecodedData(const MediaDataDecoder::DecodedData& aData,
                                  DecodedOutputIPDL& aDecodedData) override;
+  void CleanupOnActorDestroy() override;
 
  private:
+  void ReleaseUsedShmems();
   // Can only be accessed from the manager thread
   // Note: we can't keep a reference to the original AudioInfo here
   // because unlike in typical MediaDataDecoder situations, we're being
   // passed a deserialized AudioInfo from RecvPRemoteDecoderConstructor
   // which is destroyed when RecvPRemoteDecoderConstructor returns.
   const AudioInfo mAudioInfo;
+  ShmemPool mDecodedFramePool;
+  AutoTArray<ShmemBuffer, 4> mUsedShmems;
 };
 
 }  // namespace mozilla
 
 #endif  // include_dom_media_ipc_RemoteAudioDecoderChild_h
--- a/dom/media/ipc/RemoteDecoderChild.cpp
+++ b/dom/media/ipc/RemoteDecoderChild.cpp
@@ -9,22 +9,16 @@
 
 namespace mozilla {
 
 RemoteDecoderChild::RemoteDecoderChild(bool aRecreatedOnCrash)
     : mThread(RemoteDecoderManagerChild::GetManagerThread()),
       mRecreatedOnCrash(aRecreatedOnCrash),
       mRawFramePool(4) {}
 
-mozilla::ipc::IPCResult RemoteDecoderChild::RecvDoneWithInput(
-    Shmem&& aInputShmem) {
-  mRawFramePool.Put(ShmemBuffer(std::move(aInputShmem)));
-  return IPC_OK();
-}
-
 void RemoteDecoderChild::HandleRejectionError(
     const ipc::ResponseRejectReason& aReason,
     std::function<void(const MediaResult&)>&& aCallback) {
   // If the channel goes down and CanSend() returns false, the IPDL promise will
   // be rejected with SendError rather than ActorDestroyed. Both means the same
   // thing and we can consider that the parent has crashed. The child can no
   // longer be used.
 
@@ -104,50 +98,62 @@ RefPtr<MediaDataDecoder::InitPromise> Re
 
 RefPtr<MediaDataDecoder::DecodePromise> RemoteDecoderChild::Decode(
     MediaRawData* aSample) {
   AssertOnManagerThread();
 
   // TODO: It would be nice to add an allocator method to
   // MediaDataDecoder so that the demuxer could write directly
   // into shmem rather than requiring a copy here.
-  ShmemBuffer buffer = mRawFramePool.Get(this, aSample->Size());
+  ShmemBuffer buffer = mRawFramePool.Get(this, aSample->Size(),
+                                         ShmemPool::AllocationPolicy::Unsafe);
   if (!buffer.Valid()) {
     return MediaDataDecoder::DecodePromise::CreateAndReject(
         NS_ERROR_DOM_MEDIA_DECODE_ERR, __func__);
   }
 
   memcpy(buffer.Get().get<uint8_t>(), aSample->Data(), aSample->Size());
 
+  // Make a copy of the Shmem to re-use it later as IDPL will move the one used
+  // in the MediaRawDataIPDL.
+  Shmem mem(buffer.Get());
   MediaRawDataIPDL sample(
       MediaDataIPDL(aSample->mOffset, aSample->mTime, aSample->mTimecode,
                     aSample->mDuration, aSample->mKeyframe),
       aSample->mEOS, aSample->mDiscardPadding, aSample->Size(),
       std::move(buffer.Get()));
 
   RefPtr<RemoteDecoderChild> self = this;
   SendDecode(sample)->Then(
       mThread, __func__,
-      [self, this](DecodeResultIPDL&& aResponse) {
+      [self, this, mem = std::move(mem)](
+          PRemoteDecoderChild::DecodePromise::ResolveOrRejectValue&& aValue) {
+        // We no longer need the ShmemBuffer as the data has been processed by
+        // the parent.
+        mRawFramePool.Put(ShmemBuffer(mem));
+
+        if (aValue.IsReject()) {
+          self->HandleRejectionError(
+              aValue.RejectValue(), [self](const MediaResult& aError) {
+                self->mDecodePromise.RejectIfExists(aError, __func__);
+              });
+          return;
+        }
         if (mDecodePromise.IsEmpty()) {
           // We got flushed.
           return;
         }
-        if (aResponse.type() == DecodeResultIPDL::TMediaResult) {
-          mDecodePromise.Reject(aResponse.get_MediaResult(), __func__);
+        const auto& response = aValue.ResolveValue();
+        if (response.type() == DecodeResultIPDL::TMediaResult) {
+          mDecodePromise.Reject(response.get_MediaResult(), __func__);
           return;
         }
-        ProcessOutput(aResponse.get_DecodedOutputIPDL());
+        ProcessOutput(response.get_DecodedOutputIPDL());
         mDecodePromise.Resolve(std::move(mDecodedData), __func__);
         mDecodedData = MediaDataDecoder::DecodedData();
-      },
-      [self](const mozilla::ipc::ResponseRejectReason& aReason) {
-        self->HandleRejectionError(aReason, [self](const MediaResult& aError) {
-          self->mDecodePromise.RejectIfExists(aError, __func__);
-        });
       });
 
   return mDecodePromise.Ensure(__func__);
 }
 
 RefPtr<MediaDataDecoder::FlushPromise> RemoteDecoderChild::Flush() {
   AssertOnManagerThread();
   mDecodePromise.RejectIfExists(NS_ERROR_DOM_MEDIA_CANCELED, __func__);
--- a/dom/media/ipc/RemoteDecoderChild.h
+++ b/dom/media/ipc/RemoteDecoderChild.h
@@ -19,18 +19,16 @@ using mozilla::ipc::IPCResult;
 
 class RemoteDecoderChild : public PRemoteDecoderChild,
                            public IRemoteDecoderChild {
   friend class PRemoteDecoderChild;
 
  public:
   explicit RemoteDecoderChild(bool aRecreatedOnCrash = false);
 
-  IPCResult RecvDoneWithInput(Shmem&& aInputShmem);
-
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
   // IRemoteDecoderChild
   RefPtr<MediaDataDecoder::InitPromise> Init() override;
   RefPtr<MediaDataDecoder::DecodePromise> Decode(
       MediaRawData* aSample) override;
   RefPtr<MediaDataDecoder::DecodePromise> Drain() override;
   RefPtr<MediaDataDecoder::FlushPromise> Flush() override;
--- a/dom/media/ipc/RemoteDecoderParent.cpp
+++ b/dom/media/ipc/RemoteDecoderParent.cpp
@@ -13,18 +13,17 @@ namespace mozilla {
 
 using media::TimeUnit;
 
 RemoteDecoderParent::RemoteDecoderParent(RemoteDecoderManagerParent* aParent,
                                          TaskQueue* aManagerTaskQueue,
                                          TaskQueue* aDecodeTaskQueue)
     : mParent(aParent),
       mManagerTaskQueue(aManagerTaskQueue),
-      mDecodeTaskQueue(aDecodeTaskQueue),
-      mDecodedFramePool(4) {
+      mDecodeTaskQueue(aDecodeTaskQueue) {
   MOZ_COUNT_CTOR(RemoteDecoderParent);
   MOZ_ASSERT(OnManagerThread());
   // We hold a reference to ourselves to keep us alive until IPDL
   // explictly destroys us. There may still be refs held by
   // tasks, but no new ones should be added after we're
   // destroyed.
   mIPDLSelfRef = this;
 }
@@ -88,19 +87,16 @@ mozilla::ipc::IPCResult RemoteDecoderPar
   data->mOffset = aData.base().offset();
   data->mTime = aData.base().time();
   data->mTimecode = aData.base().timecode();
   data->mDuration = aData.base().duration();
   data->mKeyframe = aData.base().keyframe();
   data->mEOS = aData.eos();
   data->mDiscardPadding = aData.discardPadding();
 
-  // Send back to the child to reuse in ShmemPool
-  Unused << SendDoneWithInput(std::move(aData.buffer()));
-
   RefPtr<RemoteDecoderParent> self = this;
   mDecoder->Decode(data)->Then(
       mManagerTaskQueue, __func__,
       [self, this, resolver = std::move(aResolver)](
           MediaDataDecoder::DecodePromise::ResolveOrRejectValue&& aValue) {
         if (!self->CanRecv()) {
           // Avoid unnecessarily creating shmem objects later.
           return;
@@ -184,28 +180,22 @@ mozilla::ipc::IPCResult RemoteDecoderPar
 
 mozilla::ipc::IPCResult RemoteDecoderParent::RecvSetSeekThreshold(
     const TimeUnit& aTime) {
   MOZ_ASSERT(OnManagerThread());
   mDecoder->SetSeekThreshold(aTime);
   return IPC_OK();
 }
 
-mozilla::ipc::IPCResult RemoteDecoderParent::RecvDoneWithOutput(
-    Shmem&& aOutputShmem) {
-  mDecodedFramePool.Put(ShmemBuffer(std::move(aOutputShmem)));
-  return IPC_OK();
-}
-
 void RemoteDecoderParent::ActorDestroy(ActorDestroyReason aWhy) {
   MOZ_ASSERT(OnManagerThread());
   if (mDecoder) {
     mDecoder->Shutdown();
     mDecoder = nullptr;
   }
-  mDecodedFramePool.Cleanup(this);
+  CleanupOnActorDestroy();
 }
 
 bool RemoteDecoderParent::OnManagerThread() {
   return mParent->OnManagerThread();
 }
 
 }  // namespace mozilla
--- a/dom/media/ipc/RemoteDecoderParent.h
+++ b/dom/media/ipc/RemoteDecoderParent.h
@@ -31,35 +31,32 @@ class RemoteDecoderParent : public PRemo
   // PRemoteDecoderParent
   IPCResult RecvInit(InitResolver&& aResolver);
   IPCResult RecvDecode(const MediaRawDataIPDL& aData,
                        DecodeResolver&& aResolver);
   IPCResult RecvFlush(FlushResolver&& aResolver);
   IPCResult RecvDrain(DrainResolver&& aResolver);
   IPCResult RecvShutdown(ShutdownResolver&& aResolver);
   IPCResult RecvSetSeekThreshold(const media::TimeUnit& aTime);
-  IPCResult RecvDoneWithOutput(Shmem&& aOutputShmem);
 
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
  protected:
   virtual ~RemoteDecoderParent();
 
   bool OnManagerThread();
   void Error(const MediaResult& aError);
 
   virtual MediaResult ProcessDecodedData(
       const MediaDataDecoder::DecodedData& aDatam,
       DecodedOutputIPDL& aDecodedData) = 0;
+  virtual void CleanupOnActorDestroy() {}
 
   RefPtr<RemoteDecoderManagerParent> mParent;
   RefPtr<RemoteDecoderParent> mIPDLSelfRef;
   RefPtr<TaskQueue> mManagerTaskQueue;
   RefPtr<TaskQueue> mDecodeTaskQueue;
   RefPtr<MediaDataDecoder> mDecoder;
-
-  // Can only be accessed from the manager thread
-  ShmemPool mDecodedFramePool;
 };
 
 }  // namespace mozilla
 
 #endif  // include_dom_media_ipc_RemoteDecoderParent_h