1380568 - Remove AbstractMediaDecoder::NotifyDecodedFrames()

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Bug 1380568. P1 - store FrameStatistics in MFR.

So we can remove the use of AbstractMediaDecoder::NotifyDecodedFrames().

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

Comment 2

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

Attachment: Bug 1380568. P2 - remove AbstractMediaDecoder::NotifyDecodedFrames().

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

Comment 3

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

In order to move bug 1378295 forward, we still have some minimum changes to AndroidMediaReader/Decoder while bug 1379190 is pending.

Hi Jya,
Can you review the changes of this bug or bug 1379190 will be landed soon?

Comment 4

[Jean-Yves Avenard [:jya]]

bug 1379190 will go in very shortly...

Comment 5

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

Comment on attachment 8887366 [details]
Bug 1380568. P1 - store FrameStatistics in MFR.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158210/diff/1-2/

Comment 6

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

Comment on attachment 8887367 [details]
Bug 1380568. P2 - remove AbstractMediaDecoder::NotifyDecodedFrames().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158212/diff/1-2/

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8887366 [details]
Bug 1380568. P1 - store FrameStatistics in MFR.

https://reviewboard.mozilla.org/r/158210/#review165032

This is starting to become very cumbersome. It will be very painful to add any new decoders with all those arguments to think about...

::: dom/media/MediaFormatReader.cpp:2154
(Diff revision 2)
>      || (!decoder.mOutput.Length() && !decoder.mQueuedSamples.Length()),
>      "No frames can be demuxed or decoded while an internal seek is pending");
>  
>    // Record number of frames decoded and parsed. Automatically update the
>    // stats counters using the AutoNotifyDecoded stack-based class.
> -  AbstractMediaDecoder::AutoNotifyDecoded a(mDecoder);
> +  AbstractMediaDecoder::AutoNotifyDecoded a(mFrameStats);

how does this compile? does this change depends on another?

AutoNotifyDecoded constructor takes a AbstractMediaDecoder* as argument

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8887367 [details]
Bug 1380568. P2 - remove AbstractMediaDecoder::NotifyDecodedFrames().

https://reviewboard.mozilla.org/r/158212/#review165034

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8887366 [details]
Bug 1380568. P1 - store FrameStatistics in MFR.

https://reviewboard.mozilla.org/r/158210/#review165192

Comment 10

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

Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25aa6acbfce4
P1 - store FrameStatistics in MFR. r=jya
https://hg.mozilla.org/integration/autoland/rev/931a09ec5d0f
P2 - remove AbstractMediaDecoder::NotifyDecodedFrames(). r=jya

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25aa6acbfce4
https://hg.mozilla.org/mozilla-central/rev/931a09ec5d0f