1213176 - Make MediaFormatReader less reliant on the MediaDecoder's parent

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Jean-Yves Avenard [:jya]]

This makes it difficult to create gtest on MediaFormatReader alone.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove most MediaFormatReader dependencies on its MediaDecoder parent.

The LayersBackend can be defined at construction time, however if a parent MediaDecoder exists, the value will be overwritten by the MediaDecoderOwner value.

Comment 2

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

Comment on attachment 8671787 [details] [diff] [review]
P1. Remove most MediaFormatReader dependencies on its MediaDecoder parent.

Review of attachment 8671787 [details] [diff] [review]:
-----------------------------------------------------------------

1. DispatchKeyNeededEvent needs to check if mDecoder is null.
2. MediaDecoderReader::InitializationTask(), MediaDecoderReader::GetBuffered() and MediaDecoderReader::AsyncReadMetadata() need to check if mDecoder is null.

r+ with nits to fix.

Comment 3

[Jean-Yves Avenard [:jya]]

You mentioned on IRC on a way to fire the encrypted event without having the need of mDecoder. I was waiting to get more details on this.

Comment 4

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #2)
> Comment on attachment 8671787 [details] [diff] [review]
> P1. Remove most MediaFormatReader dependencies on its MediaDecoder parent.
> 
> Review of attachment 8671787 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 1. DispatchKeyNeededEvent needs to check if mDecoder is null.
ok

> 2. MediaDecoderReader::InitializationTask(),

ok.
Will have to remove the assert in MediaDecoderReader destructor.

> MediaDecoderReader::GetBuffered() and

this isn't called as MediaFormatReader overrides GetBuffered()

> MediaDecoderReader::AsyncReadMetadata() need to check if mDecoder is null.

it won't be called as MediaFormatReader overrides it (this bug is about making MediaFormatReader be testable without a decoder as part of bug 1194606)

Comment 5

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

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> You mentioned on IRC on a way to fire the encrypted event without having the
> need of mDecoder. I was waiting to get more details on this.

https://hg.mozilla.org/mozilla-central/file/b68eab795f9d/dom/media/MediaEventSource.h#l354
MediaEventSource is a generic and thread-safe class to implement the observer pattern to publish events across threads.

Here is an example.
https://hg.mozilla.org/mozilla-central/file/b68eab795f9d/dom/media/ogg/OggReader.cpp#l809

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove most MediaFormatReader dependencies on its MediaDecoder parent.

The LayersBackend can be defined at construction time, however if a parent MediaDecoder exists, the value will be overwritten by the MediaDecoderOwner value.

Comment 7

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

Comment on attachment 8672956 [details] [diff] [review]
P1. Remove most MediaFormatReader dependencies on its MediaDecoder parent.

Review of attachment 8672956 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaFormatReader.cpp
@@ +408,5 @@
>                                 mVideo.mTaskQueue,
>                                 mVideo.mCallback,
>                                 mHardwareAccelerationDisabled ? LayersBackend::LAYERS_NONE :
>                                                                 mLayersBackendType,
> +                               mVideoFrameContainer ? mVideoFrameContainer->GetImageContainer() : nullptr);

This expression appears twice. It might be worth a helper function like |bool HasImageContainer()|.

::: dom/media/fmp4/MP4Decoder.cpp
@@ +48,5 @@
>  {
> +  MediaDecoderReader* reader =
> +    new MediaFormatReader(this,
> +                          new MP4Demuxer(GetResource()),
> +                          GetVideoFrameContainer());

This should aligned to GetResource().

Comment 8

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

Comment on attachment 8672956 [details] [diff] [review]
P1. Remove most MediaFormatReader dependencies on its MediaDecoder parent.

Review of attachment 8672956 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaFormatReader.cpp
@@ +408,5 @@
>                                 mVideo.mTaskQueue,
>                                 mVideo.mCallback,
>                                 mHardwareAccelerationDisabled ? LayersBackend::LAYERS_NONE :
>                                                                 mLayersBackendType,
> +                               mVideoFrameContainer ? mVideoFrameContainer->GetImageContainer() : nullptr);

I mean GetImageContainer() { return mVideoFrameContainer ? mVideoFrameContainer->GetImageContainer() : nullptr; }

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove most MediaFormatReader dependencies on its MediaDecoder parent.

The LayersBackend can be defined at construction time, however if a parent MediaDecoder exists, the value will be overwritten by the MediaDecoderOwner value.

Carrying r+

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove most MediaFormatReader dependencies on its MediaDecoder parent.

The LayersBackend can be defined at construction time, however if a parent MediaDecoder exists, the value will be overwritten by the MediaDecoderOwner value.

Rebase

Comment 11

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eec7b0b16b59

going to manually check that it doesn't introduce regression in windows, in particular with hardware acceleration as last time I touched GetVideoContainer it caused DXVA to be disabled.

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove most MediaFormatReader dependencies on its MediaDecoder parent.

The LayersBackend can be defined at construction time, however if a parent MediaDecoder exists, the value will be overwritten by the MediaDecoderOwner value.

Proper rebase, had missed some few git add

Comment 13

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b7cf4bc5f3f

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove most MediaFormatReader dependencies on its MediaDecoder parent.

The LayersBackend can be defined at construction time, however if a parent MediaDecoder exists, the value will be overwritten by the MediaDecoderOwner value.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d16285849fb

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9d16285849fb

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9d16285849fb