1378295 - Remove the dependency on AbstractMediaDecoder from MediaFormatReader

Status: RESOLVED FIXED

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

Description

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

It will improve the reusability of MediaDecoderReader so it is easier to be reused outside the context of MediaDecoder (e.g. WebAudio and WebRTC).

Comment 1

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

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #0)
> It will improve the reusability of MediaDecoderReader so it is easier to be
> reused outside the context of MediaDecoder (e.g. WebAudio and WebRTC).

http://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/dom/media/webaudio/MediaBufferDecoder.cpp#191
BufferDecoder is an anti-pattern which exists so you can pass an AbstractMediaDecoder to the MediaDecoderReader constructor. However, most of its implementation are empty functions.

So MediaDecoderReader will be more reusable by removing its dependency on AbstractMediaDecoder.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Once bug 1379190 lands, the only class other than MediaFormatReader that inherits from MediaDecoderReader will be removed. Once the MediaFormatReader is the only class that inherits from MediaDecoderReader, we should look to remove MediaDecodeReader, pushing anything in there that's still needed into MediaFormatReader, and removing the rest.

Comment 3

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

http://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/dom/media/AbstractMediaDecoder.h#39

I prefer short names as long as they don't cause confusion. It would be nice to also change to name to MediaReader after the merge.

Comment 4

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

Thanks to bug 1316211, MediaDecoderReader is no more.

Comment 5

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

Attachment: Bug 1378295. P1 - remove unused MFR::mDecoder.

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

Comment 6

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

Attachment: Bug 1378295. P2 - move AutoNotifyDecoded from AbstractMediaDecoder to FrameStatistics.

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

Comment 7

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

Attachment: Bug 1378295. P3 - remove BufferDecoder.

Since now MFR doesn't need an AbstractMediaDecoder at all, we can
now remove BufferDecoder which exists as an empty shell to implement AbstractMediaDecoder.

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

Comment 8

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

Attachment: Bug 1378295. P4 - remove reference to AbstractMediaDecoder from TrackBuffersManager.cpp.

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

Comment 9

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

Attachment: Bug 1378295. P5 - remove AbstractMediaDecoder from MediaDecoder's base class.

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

Comment 10

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

Attachment: Bug 1378295. P6 - remove AbstractMediaDecoder and fix includes.

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

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8888612 [details]
Bug 1378295. P1 - remove unused MFR::mDecoder.

https://reviewboard.mozilla.org/r/159614/#review165038

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8888613 [details]
Bug 1378295. P2 - move AutoNotifyDecoded from AbstractMediaDecoder to FrameStatistics.

https://reviewboard.mozilla.org/r/159616/#review165040

::: dom/media/AbstractMediaDecoder.h
(Diff revision 1)
> -  // parsed and decoded frames. Use inside video demux & decode functions
> -  // to ensure all parsed and decoded frames are reported on all return paths.
> -  class AutoNotifyDecoded
> -  {
> -  public:
> -    explicit AutoNotifyDecoded(FrameStatistics* aFrameStats)

I missed the commit that change the AutoNotifyDecoded constructors...

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8888614 [details]
Bug 1378295. P3 - remove BufferDecoder.

https://reviewboard.mozilla.org/r/159618/#review165042

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8888615 [details]
Bug 1378295. P4 - remove reference to AbstractMediaDecoder from TrackBuffersManager.cpp.

https://reviewboard.mozilla.org/r/159620/#review165044

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8888616 [details]
Bug 1378295. P5 - remove AbstractMediaDecoder from MediaDecoder's base class.

https://reviewboard.mozilla.org/r/159622/#review165046

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8888617 [details]
Bug 1378295. P6 - remove AbstractMediaDecoder and fix includes.

https://reviewboard.mozilla.org/r/159624/#review165048

::: dom/media/MediaFormatReader.h:16
(Diff revision 1)
>  #include "mozilla/Maybe.h"
> +#include "mozilla/StateMirroring.h"
>  #include "mozilla/TaskQueue.h"
>  #include "mozilla/Mutex.h"
>  
> +#include "FrameStatistics.h"

forward declaration for FrameStatistics would be better no?

Comment 17

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

Comment on attachment 8888613 [details]
Bug 1378295. P2 - move AutoNotifyDecoded from AbstractMediaDecoder to FrameStatistics.

https://reviewboard.mozilla.org/r/159616/#review165040

> I missed the commit that change the AutoNotifyDecoded constructors...

The change is in Bug 1380568-P1.

Comment 18

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

Comment on attachment 8888617 [details]
Bug 1378295. P6 - remove AbstractMediaDecoder and fix includes.

https://reviewboard.mozilla.org/r/159624/#review165048

> forward declaration for FrameStatistics would be better no?

We have FrameStatistics::AutoNotifyDecoded at #279.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ff60bfdd40d
P1 - remove unused MFR::mDecoder. r=jya
https://hg.mozilla.org/integration/autoland/rev/ca52231d4002
P2 - move AutoNotifyDecoded from AbstractMediaDecoder to FrameStatistics. r=jya
https://hg.mozilla.org/integration/autoland/rev/5550726d24fb
P3 - remove BufferDecoder. r=jya
https://hg.mozilla.org/integration/autoland/rev/28d20f28bacb
P4 - remove reference to AbstractMediaDecoder from TrackBuffersManager.cpp. r=jya
https://hg.mozilla.org/integration/autoland/rev/adc9e2e9bebe
P5 - remove AbstractMediaDecoder from MediaDecoder's base class. r=jya
https://hg.mozilla.org/integration/autoland/rev/92ca759469c9
P6 - remove AbstractMediaDecoder and fix includes. r=jya

Comment 20

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

Thanks for the review!

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ff60bfdd40d
https://hg.mozilla.org/mozilla-central/rev/ca52231d4002
https://hg.mozilla.org/mozilla-central/rev/5550726d24fb
https://hg.mozilla.org/mozilla-central/rev/28d20f28bacb
https://hg.mozilla.org/mozilla-central/rev/adc9e2e9bebe
https://hg.mozilla.org/mozilla-central/rev/92ca759469c9