1363283 - Reduce ref-counting overhead of MediaFormatReader::HandleDemuxedSamples()

Status: NEW

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[JW Wang [:jwwang] [:jw_wang]]

http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/dom/media/MediaFormatReader.cpp#2037

We can reduce one AddRef/Release pair by transferring the ownership of |sample| for it is no longer used and will be removed by |decoder.mQueuedSamples.RemoveElementAt(0)| soon.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1363283 - Reduce ref-counting overhead of MediaFormatReader::HandleDemuxedSamples().

http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/dom/media/MediaFormatReader.cpp#2037
We can reduce one AddRef/Release pair by transferring the ownership
of |sample| for it is no longer used and will be removed by
|decoder.mQueuedSamples.RemoveElementAt(0)| soon.

http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/dom/media/platforms/PlatformDecoderModule.h#244
Note We will let Decode() take an already_AddRefed<MediaRawData> instead
so it is possible to transfer the ownership all the way down to the underlying
platform decoder across thread boundaries.

Review commit: https://reviewboard.mozilla.org/r/137386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137386/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8865766 [details]
Bug 1363283 - Reduce ref-counting overhead of MediaFormatReader::HandleDemuxedSamples().

https://reviewboard.mozilla.org/r/137386/#review140446

::: dom/media/Benchmark.cpp:308
(Diff revision 1)
>    MOZ_ASSERT(OnThread());
>    if (mFinished || mSampleIndex >= mSamples.Length()) {
>      return;
>    }
>    RefPtr<Benchmark> ref(mMainThreadState);
> -  mDecoder->Decode(mSamples[mSampleIndex])
> +  mDecoder->Decode(mSamples[mSampleIndex].forget())

Hi Gerald,
Can you review the change in Benchmark.cpp to make sure it is safe to transfer the ownership of mSamples[mSampleIndex]?

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8865766 [details]
Bug 1363283 - Reduce ref-counting overhead of MediaFormatReader::HandleDemuxedSamples().

https://reviewboard.mozilla.org/r/137386/#review140886

I don't like it.

It makes the code much more painful to read, more complicated to maintain for no proven benefit

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8865766 [details]
Bug 1363283 - Reduce ref-counting overhead of MediaFormatReader::HandleDemuxedSamples().

https://reviewboard.mozilla.org/r/137386/#review140888

I should add, that the aim was to have a shared MediaRawData between the MSE demuxer (that stores all those raw data in an array) and the MediaDataDecoder.

This would no longer be possible with this.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8865766 [details]
Bug 1363283 - Reduce ref-counting overhead of MediaFormatReader::HandleDemuxedSamples().

https://reviewboard.mozilla.org/r/137386/#review140934

(Deferring to jya's review)

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.