1128381 - Make MP4Reader on MacOSX dormant capable

Status: RESOLVED FIXED

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

Description

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

Similar to bug 1123535, we need to make the MP4Reader dormant capable on MacOSX.

This involves:
* Adding a bunch of #ifdef XP_MACOSX to the GONK/XP_WIN cases in MP4Reader to make it receive dormant notifications. I think we should probably just have a single define MP4READER_DORMANT or somesuch that we define if the platform supports going dormant.
* Ensure the MacOSX MediaDataDecoders can work inside the SharedDecoderManager.
* Implement MediaDataDecoder::ReleaseMediaResources() and AllocateMediaResources() functions. These are pretty ugly functions, but we're stuck with them in the short term until we can refactor the MSE Reader to not have sub-readers and instead have multiple demuxers and a single decoder that we can explicitly shutdown and re-initialize as needed.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Don't assert on empty extra_data

Don't assert on empty array

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Properly extract extradata from AVC1 sample

Properly handle extracting extradata from AVC1 data (where an extradata is already defined). Also properly handle pure annexB sample where no extradata is defined at all

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Handle on the fly video format change

Handle on the fly video format change.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Make AVCC wrapper work with SharedDecoderManager

make mac and ffmpeg (and eme) work with shareddecoder manager. I copied what the WMF decoder does for releasing resources. Not sure if that's required as such nor the advantage over calling Shutdown() directly.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Part5. Make AVCC wrapper be dormant capable

Make AVCC wrapped decoders (Apple VT/VDA, ffmpeg, EME) dormant capable. Works quite well.

Comment 6

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

Comment on attachment 8575216 [details] [diff] [review]
Part4. Make AVCC wrapper work with SharedDecoderManager

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

Almost...

::: dom/media/fmp4/AVCCDecoderModule.cpp
@@ +168,5 @@
>  void
>  AVCCMediaDataDecoder::ReleaseMediaResources()
>  {
>    if (mDecoder) {
> +    DebugOnly<nsresult> rv = mVideoTaskQueue->FlushAndDispatch(

This isn't right; this will call the wrapped decoder's Shutdown() function on the video task queue, not on the task queue all the other decoder functions are called on. You should be calling Shutdown() on the same task queue as all the other functions; if the decoder is using the video task queue, it is the decoder's responsibility to flush it.

That is, don't dispatch a task to call shutdown(), do it on the thread that ReleaseMediaResources() is called on.

Also we should try to remove task queue flushing from our code, as flushing task queues has been a source of bugs.

@@ +179,5 @@
>    }
>  }
>  
>  void
>  AVCCMediaDataDecoder::ReleaseDecoder()

MediaDataDecoder::ReleaseDecoder() is never called, and it can be removed - everywhere. I never had the time, but if you could do it, that'd be great. This can happen in another bug.

Comment 7

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

Comment on attachment 8575217 [details] [diff] [review]
Part5. Make AVCC wrapper be dormant capable

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

Almost...

::: dom/media/fmp4/MP4Reader.h
@@ +274,5 @@
>    bool mIndexReady;
>    Monitor mDemuxerMonitor;
>    nsRefPtr<SharedDecoderManager> mSharedDecoderManager;
>  
> +#if defined(XP_WIN) || defined(MOZ_APPLEMEDIA) || defined(MOZ_FFMPEG)

I think we should have two defines:

MP4_READER_DORMANT which is true on the platforms that we use dormant on, and

MP4_READER_DORMANT_HEURISTIC, which is true if defined(XP_WIN) || defined(MOZ_APPLEMEDIA) || defined(MOZ_FFMPEG), i.e. on platforms where we make the reader dormant when idle.

We should use those everywhere instead of re-doing the per-platform checks everywhere.

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Make AVCC wrapper work with SharedDecoderManager

Carrying r+.

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Part5. Make AVCC wrapper be dormant capable

Carrying r+. Bear with me on simplifying those #ifdef where really we don't need it. coming up next

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Part5. Make AVCC wrapper be dormant capable

Got carried away in a last minute change

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: Part6. Remove unused member functions

Remove ReleaseDecoder method as per comment 6

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: Part7. Streamline code across platforms

Remove some conditional code. Either the default implementation does nothing anyway so no need to have it conditional, or remove code where for weird reasons (if any) it wasn't called (originally added by bug 941302.)

Comment 13

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

Comment on attachment 8575688 [details] [diff] [review]
Part5. Make AVCC wrapper be dormant capable

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

::: dom/media/fmp4/MP4Reader.cpp
@@ +294,5 @@
>  };
>  #endif
>  
>  void MP4Reader::RequestCodecResource() {
> +#if !defined(MOZ_WIDGET_ANDROID)

#if defined(MP4_READER_DORMANT)

@@ +302,5 @@
>  #endif
>  }
>  
>  bool MP4Reader::IsWaitingOnCodecResource() {
> +#if !defined(MOZ_WIDGET_ANDROID)

#if defined(MP4_READER_DORMANT)

Comment 14

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

Comment on attachment 8575711 [details] [diff] [review]
Part7. Streamline code across platforms

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

Guess this makes my earlier comment about adding #if defined(MP4_READER_DORMANT) instead of !MOZ_WIDGET_ANDROID largely irrelevant. ;)

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 8575211 [details] [diff] [review]
Part2. Properly extract extradata from AVC1 sample

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

yay comments. r-me

Comment 16

[Ralph Giles (:rillian)]

r=me, I meant.

Comment 17

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac69a3a42ca
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e494819b6063
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4f569ba9c1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/690c0d515b82
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c7c3e256eb03
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d855681d0bf5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bf7e77671f8f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ac69a3a42ca
https://hg.mozilla.org/mozilla-central/rev/e494819b6063
https://hg.mozilla.org/mozilla-central/rev/cd4f569ba9c1
https://hg.mozilla.org/mozilla-central/rev/690c0d515b82
https://hg.mozilla.org/mozilla-central/rev/c7c3e256eb03
https://hg.mozilla.org/mozilla-central/rev/d855681d0bf5
https://hg.mozilla.org/mozilla-central/rev/bf7e77671f8f

Comment 19

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

Comment on attachment 8575208 [details] [diff] [review]
Don't assert on empty extra_data

Requesting blanked approval for the patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: I can't uplift bug 1141914 without the patches in this bug. The patches here make our handling of switching the resolution of video playback better, though they aren't required for YouTubbe (on Windows) to stream switch correctly, they are required for EME partner sites.
[Describe test coverage new/current, TreeHerder]: Local testing. We're working on a stream switching mochitest too.
[Risks and why]: Could regress MacOSX video support potentially, but it's been pretty well tested locally.
[String/UUID change made/needed]: None.

Comment 20

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

Comment on attachment 8575211 [details] [diff] [review]
Part2. Properly extract extradata from AVC1 sample

Requesting blanked approval for the patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: I can't uplift bug 1141914 without the patches in this bug. The patches here make our handling of switching the resolution of video playback better, though they aren't required for YouTubbe (on Windows) to stream switch correctly, they are required for EME partner sites.
[Describe test coverage new/current, TreeHerder]: Local testing. We're working on a stream switching mochitest too.
[Risks and why]: Could regress MacOSX video support potentially, but it's been pretty well tested locally.
[String/UUID change made/needed]: None.

Comment 21

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

Comment on attachment 8575214 [details] [diff] [review]
Part3. Handle on the fly video format change

Requesting blanked approval for the patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: I can't uplift bug 1141914 without the patches in this bug. The patches here make our handling of switching the resolution of video playback better, though they aren't required for YouTubbe (on Windows) to stream switch correctly, they are required for EME partner sites.
[Describe test coverage new/current, TreeHerder]: Local testing. We're working on a stream switching mochitest too.
[Risks and why]: Could regress MacOSX video support potentially, but it's been pretty well tested locally.
[String/UUID change made/needed]: None.

Comment 22

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

Comment on attachment 8575681 [details] [diff] [review]
Part4. Make AVCC wrapper work with SharedDecoderManager

Requesting blanked approval for the patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: I can't uplift bug 1141914 without the patches in this bug. The patches here make our handling of switching the resolution of video playback better, though they aren't required for YouTubbe (on Windows) to stream switch correctly, they are required for EME partner sites.
[Describe test coverage new/current, TreeHerder]: Local testing. We're working on a stream switching mochitest too.
[Risks and why]: Could regress MacOSX video support potentially, but it's been pretty well tested locally.
[String/UUID change made/needed]: None.

Comment 23

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

Comment on attachment 8575699 [details] [diff] [review]
Part5. Make AVCC wrapper be dormant capable

Requesting blanked approval for the patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: I can't uplift bug 1141914 without the patches in this bug. The patches here make our handling of switching the resolution of video playback better, though they aren't required for YouTubbe (on Windows) to stream switch correctly, they are required for EME partner sites.
[Describe test coverage new/current, TreeHerder]: Local testing. We're working on a stream switching mochitest too.
[Risks and why]: Could regress MacOSX video support potentially, but it's been pretty well tested locally.
[String/UUID change made/needed]: None.

Comment 24

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

Comment on attachment 8575700 [details] [diff] [review]
Part6. Remove unused member functions

Requesting blanked approval for the patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: I can't uplift bug 1141914 without the patches in this bug. The patches here make our handling of switching the resolution of video playback better, though they aren't required for YouTubbe (on Windows) to stream switch correctly, they are required for EME partner sites.
[Describe test coverage new/current, TreeHerder]: Local testing. We're working on a stream switching mochitest too.
[Risks and why]: Could regress MacOSX video support potentially, but it's been pretty well tested locally.
[String/UUID change made/needed]: None.

Comment 25

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

Comment on attachment 8575711 [details] [diff] [review]
Part7. Streamline code across platforms

Requesting blanked approval for the patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: I can't uplift bug 1141914 without the patches in this bug. The patches here make our handling of switching the resolution of video playback better, though they aren't required for YouTubbe (on Windows) to stream switch correctly, they are required for EME partner sites.
[Describe test coverage new/current, TreeHerder]: Local testing. We're working on a stream switching mochitest too.
[Risks and why]: Could regress MacOSX video support potentially, but it's been pretty well tested locally.
[String/UUID change made/needed]: None.

Comment 26

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

Must remember to uplift to EME.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

This is pretty significant work so tracking to keep an eye on it. We can take this up to Aurora now but please get tests ready for when 38 hits Beta.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4e35ffca555e
https://hg.mozilla.org/releases/mozilla-aurora/rev/f3d0d5ada680
https://hg.mozilla.org/releases/mozilla-aurora/rev/c4da71ceaf4c
https://hg.mozilla.org/releases/mozilla-aurora/rev/2ad9bd374080
https://hg.mozilla.org/releases/mozilla-aurora/rev/408e6acc619e
https://hg.mozilla.org/releases/mozilla-aurora/rev/958a39180eda
https://hg.mozilla.org/releases/mozilla-aurora/rev/b5ecfb63fbd8