Closed Bug 1213176 Opened 5 years ago Closed 5 years ago

Make MediaFormatReader less reliant on the MediaDecoder's parent

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(1 file, 5 obsolete files)

This makes it difficult to create gtest on MediaFormatReader alone.
Assignee: nobody → jyavenard
The LayersBackend can be defined at construction time, however if a parent MediaDecoder exists, the value will be overwritten by the MediaDecoderOwner value.
Attachment #8671787 - Flags: review?(jwwang)
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.
Attachment #8671787 - Flags: review?(jwwang) → review+
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.
(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)
(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
The LayersBackend can be defined at construction time, however if a parent MediaDecoder exists, the value will be overwritten by the MediaDecoderOwner value.
Attachment #8672956 - Flags: review?(jwwang)
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().
Attachment #8672956 - Flags: review?(jwwang) → review+
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; }
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+
Attachment #8673022 - Flags: superreview+
Attachment #8671787 - Attachment is obsolete: true
Attachment #8672956 - Attachment is obsolete: true
Priority: -- → P2
The LayersBackend can be defined at construction time, however if a parent MediaDecoder exists, the value will be overwritten by the MediaDecoderOwner value.

Rebase
Attachment #8674065 - Flags: review+
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.
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
Attachment #8674067 - Flags: review+
Attachment #8673022 - Attachment is obsolete: true
Attachment #8674065 - Attachment is obsolete: true
The LayersBackend can be defined at construction time, however if a parent MediaDecoder exists, the value will be overwritten by the MediaDecoderOwner value.
Attachment #8674130 - Flags: review+
Attachment #8674067 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9d16285849fb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.