Make MP4Reader on MacOSX dormant capable

RESOLVED FIXED in Firefox 38

Status

()

Core
Audio/Video
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpearce, Assigned: jya)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38+ fixed, firefox39 fixed)

Details

Attachments

(7 attachments, 3 obsolete attachments)

1.64 KB, patch
kentuckyfriedtakahe
: 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
(Reporter)

Description

3 years ago
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.

Updated

3 years ago
Priority: -- → P2
(Assignee)

Updated

3 years ago
Depends on: 1139779
(Assignee)

Comment 1

3 years ago
Created attachment 8575208 [details] [diff] [review]
Don't assert on empty extra_data

Don't assert on empty array
Attachment #8575208 - Flags: review?(ajones)
(Assignee)

Comment 2

3 years ago
Created attachment 8575211 [details] [diff] [review]
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
Attachment #8575211 - Flags: review?(giles)
(Assignee)

Comment 3

3 years ago
Created attachment 8575214 [details] [diff] [review]
Part3. Handle on the fly video format change

Handle on the fly video format change.
Attachment #8575214 - Flags: review?(cpearce)
(Assignee)

Comment 4

3 years ago
Created attachment 8575216 [details] [diff] [review]
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.
Attachment #8575216 - Flags: review?(cpearce)
(Assignee)

Comment 5

3 years ago
Created attachment 8575217 [details] [diff] [review]
Part5. Make AVCC wrapper be dormant capable

Make AVCC wrapped decoders (Apple VT/VDA, ffmpeg, EME) dormant capable. Works quite well.
Attachment #8575217 - Flags: review?(cpearce)
(Reporter)

Updated

3 years ago
Attachment #8575214 - Flags: review?(cpearce) → review+
(Reporter)

Comment 6

3 years ago
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-
(Reporter)

Comment 7

3 years ago
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+
(Assignee)

Comment 8

3 years ago
Created attachment 8575681 [details] [diff] [review]
Part4. Make AVCC wrapper work with SharedDecoderManager

Carrying r+.
Attachment #8575681 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Attachment #8575216 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Created attachment 8575688 [details] [diff] [review]
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
Attachment #8575688 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Attachment #8575217 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
Created attachment 8575699 [details] [diff] [review]
Part5. Make AVCC wrapper be dormant capable

Got carried away in a last minute change
Attachment #8575699 - Flags: review?(cpearce)
(Assignee)

Comment 11

3 years ago
Created attachment 8575700 [details] [diff] [review]
Part6. Remove unused member functions

Remove ReleaseDecoder method as per comment 6
Attachment #8575700 - Flags: review?(cpearce)
(Assignee)

Comment 12

3 years ago
Created attachment 8575711 [details] [diff] [review]
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.)
Attachment #8575711 - Flags: review?(cpearce)
(Reporter)

Updated

3 years ago
Attachment #8575681 - Flags: review?(cpearce) → review+
(Reporter)

Comment 13

3 years ago
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+
(Reporter)

Updated

3 years ago
Attachment #8575700 - Flags: review?(cpearce) → review+
(Reporter)

Comment 14

3 years ago
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+
(Assignee)

Updated

3 years ago
Attachment #8575688 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8575699 - Flags: review?(cpearce) → review+
(Assignee)

Updated

3 years ago
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.
(Reporter)

Comment 19

3 years ago
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?
(Reporter)

Comment 20

3 years ago
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?
(Reporter)

Comment 21

3 years ago
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?
(Reporter)

Comment 22

3 years ago
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?
(Reporter)

Comment 23

3 years ago
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?
(Reporter)

Comment 24

3 years ago
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?
(Reporter)

Comment 25

3 years ago
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?
(Reporter)

Comment 26

3 years ago
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.
status-firefox38: --- → affected
tracking-firefox38: --- → +
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+
(Reporter)

Updated

3 years ago
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.