Closed Bug 1128381 Opened 9 years ago Closed 9 years ago

Make MP4Reader on MacOSX dormant capable

Categories

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

x86_64
Windows 8.1
defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 + fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 3 obsolete files)

1.64 KB, patch
ajones
: review+
Details | Diff | Splinter Review
4.23 KB, patch
rillian
: review+
Details | Diff | Splinter Review
5.40 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
3.40 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
4.12 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
5.32 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
2.07 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
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.
Priority: -- → P2
Depends on: 1139779
Don't assert on empty array
Attachment #8575208 - Flags: review?(ajones)
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
Attachment #8575211 - Flags: review?(giles)
Handle on the fly video format change.
Attachment #8575214 - Flags: review?(cpearce)
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.
Attachment #8575216 - Flags: review?(cpearce)
Make AVCC wrapped decoders (Apple VT/VDA, ffmpeg, EME) dormant capable. Works quite well.
Attachment #8575217 - Flags: review?(cpearce)
Attachment #8575214 - Flags: review?(cpearce) → review+
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.
Attachment #8575216 - Flags: review?(cpearce) → review-
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.
Attachment #8575217 - Flags: review?(cpearce) → review-
Attachment #8575208 - Flags: review?(ajones) → review+
Carrying r+.
Attachment #8575681 - Flags: review?(cpearce)
Attachment #8575216 - Attachment is obsolete: true
Carrying r+. Bear with me on simplifying those #ifdef where really we don't need it. coming up next
Attachment #8575688 - Flags: review?(cpearce)
Attachment #8575217 - Attachment is obsolete: true
Got carried away in a last minute change
Attachment #8575699 - Flags: review?(cpearce)
Remove ReleaseDecoder method as per comment 6
Attachment #8575700 - Flags: review?(cpearce)
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.)
Attachment #8575711 - Flags: review?(cpearce)
Attachment #8575681 - Flags: review?(cpearce) → review+
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)
Attachment #8575688 - Flags: review?(cpearce) → review+
Attachment #8575700 - Flags: review?(cpearce) → review+
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. ;)
Attachment #8575711 - Flags: review?(cpearce) → review+
Attachment #8575688 - Attachment is obsolete: true
Attachment #8575699 - Flags: review?(cpearce) → review+
Blocks: 1141914
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
Attachment #8575211 - Flags: review?(giles) → review+
r=me, I meant.
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.
Attachment #8575208 - Flags: approval-mozilla-aurora?
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.
Attachment #8575211 - Flags: approval-mozilla-aurora?
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.
Attachment #8575214 - Flags: approval-mozilla-aurora?
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.
Attachment #8575681 - Flags: approval-mozilla-aurora?
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.
Attachment #8575699 - Flags: approval-mozilla-aurora?
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.
Attachment #8575700 - Flags: approval-mozilla-aurora?
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.
Attachment #8575711 - Flags: approval-mozilla-aurora?
Must remember to uplift to EME.
Flags: needinfo?(cpearce)
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.
Attachment #8575208 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8575211 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8575214 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8575681 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8575699 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8575700 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8575711 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: