Open Bug 1295921 Opened 8 years ago Updated 2 years ago

Don't suspend video decoder for elements as sources to drawImage()

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- affected

People

(Reporter: kaku, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(17 files, 1 obsolete file)

Bug 1295921 - P1: Track decoder tainting.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1295921 - P2: Mark element tainted when DrawImage is used.
58 bytes, text/x-review-board-request
jwwang
: review+
mattwoodrow
: review+
Details
Bug 1295921 - P3: Test element becomes tainted by DrawImage.
58 bytes, text/x-review-board-request
jwwang
: review+
smaug
: review+
Details
Bug 1295921 - P4: Clean up suspend timer canceling.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1295921 - P5: Test video suspend canceling.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1295921 - P6: Move resume from suspend into new function.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1295921 - P7: Test drawImage gets a non-white image from suspended video.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1295921 - P9: NullDecoderModule returns null VideoData.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1295921 - PA: Return blank decoder to being green.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1295921 - PB: Implement dispatching of pending main thread MediaDecoder tasks.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
debug-1.patch
683 bytes, patch
Details | Diff | Splinter Review
debug-2.patch
845 bytes, patch
Details | Diff | Splinter Review
spin-main-thread.patch
2.57 KB, patch
Details | Diff | Splinter Review
make-sure-drawTarget-exist.patch
1.36 KB, patch
Details | Diff | Splinter Review
Bug 1295921 - PD: Keep last keyframe in MFR.
58 bytes, text/x-review-board-request
jya
: review+
Details 
Just like bug 1284389, if a video element is a source of drawImage() or createImageBitmap(), we should not suspend its decoder.
What if the decoder is suspended before drawImage()?
This is a problem, resuming decoder is an async operation with latency. So, while drawing an already-suspended-video onto a canvas, there must be several "blank" frames been drawn, even though we resume the decoder immediately.

To completely solve this problem, we must make "resuming decoder" a blocking operation, however, I don't think this is a good way. 

Thoughts?
Flags: needinfo?(jwwang)
Flags: needinfo?(dglastonbury)
This issue should also apply to bug 1284389.
I'll have a think about this some more.
Assignee: nobody → dglastonbury
Status: NEW → ASSIGNED
Flags: needinfo?(dglastonbury)
There are three scenarios to consider:

  1. The media element is used in drawImage() before video is suspended,
  2. The media element is used in drawImage() as the video is suspending,
  3. The media element is used in drawImage() when the video is suspended.

To tackle this, I propose to `taint` any media elements that have been used by drawImage(). This taint will mark the media element as unable to participate in video decoder suspending.

This is enough for 1.
For 2. the pending suspend needs to be cancelled.
For 3. the video decoder will need to be restarted and pump out a frame before continuing.

drawImage() works by pulling the image from the front of ImageContainer. (nsLayoutUtils::SurfaceForElement). For 3. this will require blocking until the image flows through the pipeline.
(In reply to Dan Glastonbury :kamidphish from comment #5)
> drawImage() works by pulling the image from the front of ImageContainer.
> (nsLayoutUtils::SurfaceForElement). For 3. this will require blocking until
> the image flows through the pipeline.
I have one more concern, what if the script invoke the drawImage() while the video element's playback is at time _x_ and it takes time _t_ to resume the decoder with playback keeps going, so that the first fame arrived is at time ( _x_ + _t_ ). In this case, will we draw the frame at time ( _x_ + _t_ ) onto the canvas?
Blake told me (briefly) that while he was at NZ last week, someone proposed to have JS interpreter tell us that the script might use {drawImage(), captureStream() or createImageBitmap()}, then we could disable suspend-video-decoer in that web page. Is it a good idea? does it overwhelm the JS engine?

By the way, how about let's have a telemetry to see how often a HIDDEN video element is used as the source of {drawImage(), captureStream() and createImageBitmap()}?
Let the number speaks. We don't need to worry too much about the rare cases since they have less impact on the users.
Flags: needinfo?(jwwang)
Depends on: 1299718
Blocks: Phase_1_Shutdown_Decoder
(In reply to Dan Glastonbury :kamidphish from comment #17)
> Try Run -
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9db08ec6193d

Failed because I didn't capture self by value!
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review77702

::: layout/base/nsLayoutUtils.cpp:7496
(Diff revision 2)
>    // If it doesn't have a principal, just bail
>    nsCOMPtr<nsIPrincipal> principal = aElement->GetCurrentVideoPrincipal();
>    if (!principal)
>      return result;
>  
> +  if (aElement->IsVideoDecodeSuspended()) {

This seems like it's exposing media internal details to the callers.

Couldn't we instead make the semantics of aElement->GetImageContainer() that it always:
 * Marks the decoder as tainted
 * Blocks until the next frame if the decoder was suspended.

Then you could add an alternate overload for GetImageContainer with a scarier name (GetImageContainerWithoutUpdate?) so that media-internal callers that don't want the 'obvious' behaviour can still have it.

Alternatively just rename GetImageContainer to something scary, and add a function to grab the locked image and bypass ImageContainer entirely.
Comment on attachment 8791475 [details]
Bug 1295921 - P1: Track decoder tainting.

https://reviewboard.mozilla.org/r/78904/#review77712

::: dom/media/MediaDecoder.h:385
(Diff revision 1)
>  
>    // Force override the visible state to hidden.
>    // Called from HTMLMediaElement when testing of video decode suspend from mochitests.
>    void SetForcedHidden(bool aForcedHidden);
>  
> +  // Mark the decoder as tainted, such that when aTainted is true, it can't

Could you add a comment on what tainted means and in which context it is used?

::: dom/media/MediaDecoder.h:833
(Diff revision 1)
>    Canonical<bool> mMediaSeekableOnlyInBufferedRanges;
>  
>    // True if the decoder is visible.
>    Canonical<bool> mIsVisible;
>  
> +  // True is the decoder has a suspend taint - meaning a behaviour was

I think we're supposed to use US english
Attachment #8791475 - Flags: review+
Comment on attachment 8791476 [details]
Bug 1295921 - P2: Mark element tainted when DrawImage is used.

https://reviewboard.mozilla.org/r/78906/#review77714

r+ on the proviso that this is correct code.
But I'm not sure that the approach is sound.
It will only work if drawImage is called before the element going into the background. And when drawImage will be called is something that no one can assume.

Other that checking the full JS code itself, I'm not sure there's a way though and my guess this is rare enough that it probably doesn't matter much anyway
Attachment #8791476 - Flags: review+
Comment on attachment 8791477 [details]
Bug 1295921 - P3: Test element becomes tainted by DrawImage.

https://reviewboard.mozilla.org/r/78908/#review77716

::: dom/webidl/HTMLMediaElement.webidl:231
(Diff revision 1)
> +
> +/*
> + * This is an API for querying the element to determine if it has been tainted
> + * to exclude it from participating in video decoder suspending.
> + */
> +partial interface HTMLMediaElement {

You'll need DOM peer review for this...

But why was this defined outside the main HTMLMediaElement webidl ?
Comment on attachment 8791477 [details]
Bug 1295921 - P3: Test element becomes tainted by DrawImage.

https://reviewboard.mozilla.org/r/78908/#review77718
Attachment #8791477 - Flags: review+
Comment on attachment 8791478 [details]
Bug 1295921 - P4: Clean up suspend timer canceling.

https://reviewboard.mozilla.org/r/78910/#review77720

::: dom/media/MediaDecoderStateMachine.cpp:1783
(Diff revision 1)
>      TimeStamp target = TimeStamp::Now() + SuspendBackgroundVideoDelay();
>  
>      RefPtr<MediaDecoderStateMachine> self = this;
>      mVideoDecodeSuspendTimer.Ensure(target,
>                                      [=]() { self->OnSuspendTimerResolved(); },
> -                                    [=]() { self->OnSuspendTimerRejected(); });
> +                                    [=]() { MOZ_DIAGNOSTIC_ASSERT(false); });

Why wouldn't this ever occur following those changes?

this is in need of a comment (or additional text in assert)

::: dom/media/MediaDecoderStateMachine.cpp:3148
(Diff revision 1)
> -MediaDecoderStateMachine::OnSuspendTimerRejected()
> +MediaDecoderStateMachine::CancelSuspendTimer()
>  {
> -  DECODER_LOG("OnSuspendTimerRejected");
> +  DECODER_LOG("CancelSuspendTimer");
>    MOZ_ASSERT(OnTaskQueue());
>    MOZ_ASSERT(!mVideoDecodeSuspended);
> -  mVideoDecodeSuspendTimer.CompleteRequest();
> +  if (mVideoDecodeSuspendTimer.IsScheduled()) {

How couldn't it not be scheduled if we need to cancel the timer?

An assert that one is always scheduled seems more appriopriate to me.
Attachment #8791478 - Flags: review+
Comment on attachment 8791479 [details]
Bug 1295921 - P5: Test video suspend canceling.

https://reviewboard.mozilla.org/r/78912/#review77722
Attachment #8791479 - Flags: review+
Comment on attachment 8791480 [details]
Bug 1295921 - P6: Move resume from suspend into new function.

https://reviewboard.mozilla.org/r/78914/#review77724

::: dom/media/MediaDecoderStateMachine.cpp:3165
(Diff revision 1)
>  {
>    DECODER_LOG("OnSuspendTimerResolved");
>    mVideoDecodeSuspendTimer.CompleteRequest();
>    mVideoDecodeSuspended = true;
> -  mOnPlaybackEvent.Notify(MediaEventType::EnterVideoSuspend);
>    mReader->SetVideoBlankDecode(true);

You do the opposite action in the watcher.

I think you should either remove the watcher as it's unecessary wrapping.
Or make all the actions (that is the call to mReader->SetVideoBlankDecode(XX) by the watcher instead to keep consistency.
Attachment #8791480 - Flags: review+
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review77726

::: dom/html/HTMLMediaElement.cpp:845
(Diff revision 2)
> +
> +already_AddRefed<layers::Image>
> +HTMLMediaElement::MarkSuspendTaintAndGetNextFrame()
> +{
> +  RefPtr<layers::Image> image;
> +  if (mDecoder) {

if (!mDecoder) return nullptr; right at the top so you don't need to create image refptr when it won't be used

::: dom/media/MediaDecoderStateMachine.cpp:1866
(Diff revision 2)
> +  // resumed *before* the taint is set.
> +  MOZ_ASSERT(!mVideoDecodeSuspended);
> +}
> +
> +void
> +MediaDecoderStateMachine::ResumeVideoDecodeAndGetNextFrame(Monitor& monitor, RefPtr<VideoData>& result)

formatting to 80 columns
Attachment #8791481 - Flags: review+
Comment on attachment 8791482 [details]
Bug 1295921 - P7: Test drawImage gets a non-white image from suspended video.

https://reviewboard.mozilla.org/r/78918/#review77728
Attachment #8791482 - Flags: review+
Comment on attachment 8791475 [details]
Bug 1295921 - P1: Track decoder tainting.

https://reviewboard.mozilla.org/r/78904/#review77936

::: dom/media/MediaDecoder.h:833
(Diff revision 1)
>    Canonical<bool> mMediaSeekableOnlyInBufferedRanges;
>  
>    // True if the decoder is visible.
>    Canonical<bool> mIsVisible;
>  
> +  // True is the decoder has a suspend taint - meaning a behaviour was

Could use a more concise comment.

True 'if' the decoder has... - meaning video decode suspend is disabled.
Attachment #8791475 - Flags: review?(jwwang) → review+
Comment on attachment 8791476 [details]
Bug 1295921 - P2: Mark element tainted when DrawImage is used.

https://reviewboard.mozilla.org/r/78906/#review77938
Attachment #8791476 - Flags: review?(jwwang) → review+
Comment on attachment 8791477 [details]
Bug 1295921 - P3: Test element becomes tainted by DrawImage.

https://reviewboard.mozilla.org/r/78908/#review77940

::: dom/webidl/HTMLMediaElement.webidl:231
(Diff revision 1)
> +
> +/*
> + * This is an API for querying the element to determine if it has been tainted
> + * to exclude it from participating in video decoder suspending.
> + */
> +partial interface HTMLMediaElement {

It is time to have an umbrella pref (ex: media.test.video-suspend) so we don't have to define a new pref for each test-only function.
Attachment #8791477 - Flags: review?(jwwang) → review+
Comment on attachment 8791478 [details]
Bug 1295921 - P4: Clean up suspend timer canceling.

https://reviewboard.mozilla.org/r/78910/#review77942

::: dom/media/MediaDecoderStateMachine.cpp:1555
(Diff revision 1)
>  
>    mQueuedSeek.RejectIfExists(__func__);
>  
>    DiscardSeekTaskIfExist();
>  
>    // Shutdown happens will decode timer is active, we need to disconnect and

Shutdown happens 'when' ...

::: dom/media/MediaDecoderStateMachine.cpp:1783
(Diff revision 1)
>      TimeStamp target = TimeStamp::Now() + SuspendBackgroundVideoDelay();
>  
>      RefPtr<MediaDecoderStateMachine> self = this;
>      mVideoDecodeSuspendTimer.Ensure(target,
>                                      [=]() { self->OnSuspendTimerResolved(); },
> -                                    [=]() { self->OnSuspendTimerRejected(); });
> +                                    [=]() { MOZ_DIAGNOSTIC_ASSERT(false); });

No need to capture 'self'.

::: dom/media/MediaDecoderStateMachine.cpp:3148
(Diff revision 1)
> -MediaDecoderStateMachine::OnSuspendTimerRejected()
> +MediaDecoderStateMachine::CancelSuspendTimer()
>  {
> -  DECODER_LOG("OnSuspendTimerRejected");
> +  DECODER_LOG("CancelSuspendTimer");
>    MOZ_ASSERT(OnTaskQueue());
>    MOZ_ASSERT(!mVideoDecodeSuspended);
> -  mVideoDecodeSuspendTimer.CompleteRequest();
> +  if (mVideoDecodeSuspendTimer.IsScheduled()) {

To jya:
Shutdown() will call this function even when the timer is not scheduled at all.
Attachment #8791478 - Flags: review?(jwwang) → review+
Comment on attachment 8791479 [details]
Bug 1295921 - P5: Test video suspend canceling.

https://reviewboard.mozilla.org/r/78912/#review77944

::: dom/media/test/test_background_video_cancel_suspend_taint.html:46
(Diff revision 1)
> +    let v = appendVideoToDoc(test.name, token);
> +    manager.started(token);
> +
> +    let start;
> +    waitUntilPlaying(v)
> +      .then(() => { v.setVisible(false); return waitTil(v, v.duration / 2); })

We need |v.duration/2 < 10000| for this test case to be valid. Please add a test so we won't add files with improper duration in the future.
Attachment #8791479 - Flags: review?(jwwang) → review+
Comment on attachment 8791480 [details]
Bug 1295921 - P6: Move resume from suspend into new function.

https://reviewboard.mozilla.org/r/78914/#review77958

::: dom/media/MediaDecoderStateMachine.cpp:1805
(Diff revision 1)
> +              mIsVisible ? 'T' : 'F',
> +              mVideoDecodeSuspended ? 'T' : 'F',
> +              mIsReaderSuspended ? 'T' : 'F');
>  
>    if (mVideoDecodeSuspended) {
> -    mVideoDecodeSuspended = false;
> +      mOnPlaybackEvent.Notify(MediaEventType::EnterVideoSuspend);

2 spaces indentation.

::: dom/media/MediaDecoderStateMachine.cpp:1807
(Diff revision 1)
> +              mIsReaderSuspended ? 'T' : 'F');
>  
>    if (mVideoDecodeSuspended) {
> -    mVideoDecodeSuspended = false;
> +      mOnPlaybackEvent.Notify(MediaEventType::EnterVideoSuspend);
> +  }
> +  else {

coding style:
if () {
} else {
}

::: dom/media/MediaDecoderStateMachine.cpp:3165
(Diff revision 1)
>  {
>    DECODER_LOG("OnSuspendTimerResolved");
>    mVideoDecodeSuspendTimer.CompleteRequest();
>    mVideoDecodeSuspended = true;
> -  mOnPlaybackEvent.Notify(MediaEventType::EnterVideoSuspend);
>    mReader->SetVideoBlankDecode(true);

This line should be moved into VideoDecodeSuspendChanged() as well.
Attachment #8791480 - Flags: review?(jwwang) → review+
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review77964

::: dom/media/MediaDecoderStateMachine.cpp:1871
(Diff revision 2)
> +MediaDecoderStateMachine::ResumeVideoDecodeAndGetNextFrame(Monitor& monitor, RefPtr<VideoData>& result)
> +{
> +  MOZ_ASSERT(OnTaskQueue());
> +
> +  mVideoCallback.DisconnectIfExists();
> +  mReader->ResetDecode(TrackInfo::kVideoTrack);

Why calling ResetDecode()?

::: dom/media/MediaDecoderStateMachine.cpp:1875
(Diff revision 2)
> +  mVideoCallback.DisconnectIfExists();
> +  mReader->ResetDecode(TrackInfo::kVideoTrack);
> +
> +  bool invoked = false;
> +  RefPtr<MediaDecoderStateMachine> self = this;
> +  mResumeVideoPushCallback =

This listener might get a frame produced by the blank decoder, right?

::: dom/media/MediaDecoderStateMachine.cpp:1877
(Diff revision 2)
> +
> +  bool invoked = false;
> +  RefPtr<MediaDecoderStateMachine> self = this;
> +  mResumeVideoPushCallback =
> +    VideoQueue().PushEvent().Connect(OwnerThread(),
> +      [&, self, invoked](RefPtr<MediaData> newData) mutable {

This will not work as intended. The lambda will be copied multiple times when passing to MediaEventSource. Each copied lambda will get a pristine |invoked| which is initialized to false.

::: dom/media/MediaDecoderStateMachine.cpp:1906
(Diff revision 2)
> +  Monitor monitor("BlockAndResumeVideoDecodeAndGetNextFrame");
> +  RefPtr<VideoData> result;
> +
> +  // Dispatch to task queue to restart video decoder and wait for the first
> +  // frame to be produced...
> +  OwnerThread()->Dispatch(

This will cause event out of order. Please refer TaskQueue::BeginShutdown() for how to dispatch tasks in the tail dispatcher queue immediately without waiting for the tail dispatching phase.

Also, you need to drain direct tasks before doing that to enusre events are exectued in the order that are dispatched.

::: layout/base/nsLayoutUtils.cpp:7496
(Diff revision 2)
>    // If it doesn't have a principal, just bail
>    nsCOMPtr<nsIPrincipal> principal = aElement->GetCurrentVideoPrincipal();
>    if (!principal)
>      return result;
>  
> +  if (aElement->IsVideoDecodeSuspended()) {

This check is racy because even if it returns false, video-suspend might be happening off the main thread.

::: layout/base/nsLayoutUtils.cpp:7499
(Diff revision 2)
>      return result;
>  
> +  if (aElement->IsVideoDecodeSuspended()) {
> +    // The decoder is suspended.  Need to block this operation until the media
> +    // decoder can 'wakeup' and produce the correct frame.
> +    result.mLayersImage = aElement->MarkSuspendTaintAndGetNextFrame();

I don't like this approach either. Maybe we can do:
1. tell the media element to resume video decoding if necessary. And then...
2. tell the image container to wait until a valid frame can be retrieved.

Since ImageContainer has an internal monitor, we should be able to use it for synchronization between threads.
Attachment #8791481 - Flags: review?(jwwang) → review-
Comment on attachment 8791482 [details]
Bug 1295921 - P7: Test drawImage gets a non-white image from suspended video.

https://reviewboard.mozilla.org/r/78918/#review77976

::: dom/media/test/test_background_video_drawimage_with_suspended_video.html:38
(Diff revision 2)
> +
> +  gfx.drawImage(v, 0, 0, 4, 4);
> +  let imageData = gfx.getImageData(0, 0, 4, 4);
> +  let pixels = imageData.data;
> +  for (let i = 0; i < 4*4; i++) {
> +    ok(pixels[4*i+0] != 255 && pixels[4*i+1] != 255 && pixels[4*i+2] != 255,

Add a comment to note that this test assumes the pixels produced by the blank decoder are 255 since changes in the blank decoder might break the test.
Attachment #8791482 - Flags: review?(jwwang) → review+
Comment on attachment 8791480 [details]
Bug 1295921 - P6: Move resume from suspend into new function.

https://reviewboard.mozilla.org/r/78914/#review77724

> You do the opposite action in the watcher.
> 
> I think you should either remove the watcher as it's unecessary wrapping.
> Or make all the actions (that is the call to mReader->SetVideoBlankDecode(XX) by the watcher instead to keep consistency.

I moved code into the VideoDecodeSuspendChanged() for consistency.
Comment on attachment 8791480 [details]
Bug 1295921 - P6: Move resume from suspend into new function.

https://reviewboard.mozilla.org/r/78914/#review77958

> coding style:
> if () {
> } else {
> }

yeah, but...
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review77702

> This seems like it's exposing media internal details to the callers.
> 
> Couldn't we instead make the semantics of aElement->GetImageContainer() that it always:
>  * Marks the decoder as tainted
>  * Blocks until the next frame if the decoder was suspended.
> 
> Then you could add an alternate overload for GetImageContainer with a scarier name (GetImageContainerWithoutUpdate?) so that media-internal callers that don't want the 'obvious' behaviour can still have it.
> 
> Alternatively just rename GetImageContainer to something scary, and add a function to grab the locked image and bypass ImageContainer entirely.

I changed the code to not expose all these internals of HTMLMediaElement and decoders to layers. I moved the foundation work for that in to P2, so please review that once I update the patches.
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review77964

> Why calling ResetDecode()?

To flush out any pending frames from the blank decoder. Ideally, we would never add the blank frames to the decoder and expire frames in the past.

> This listener might get a frame produced by the blank decoder, right?

No because I call ResetDecode().

> This will not work as intended. The lambda will be copied multiple times when passing to MediaEventSource. Each copied lambda will get a pristine |invoked| which is initialized to false.

What is want is the final copy of the lambda to have some state and I'm using lambda capture to allocate that variable. In the lambda to catch if the event is called more than once. (I thought there might be race between disconnecting the event listener and another frame being pushed, although I haven't seen that in practice).

Is there only one final copy of the lambda? Or are you saying that multiple independent lambdas are stored?
Because if the copies are only temporary, I don't see how that is an issue.

> This will cause event out of order. Please refer TaskQueue::BeginShutdown() for how to dispatch tasks in the tail dispatcher queue immediately without waiting for the tail dispatching phase.
> 
> Also, you need to drain direct tasks before doing that to enusre events are exectued in the order that are dispatched.

OK.
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review77964

> No because I call ResetDecode().

There is no way to cancel a job running on another thread perfectly. You still have a chance to get a frame produced by the blank decoder even after ResetDecode() is called.

> What is want is the final copy of the lambda to have some state and I'm using lambda capture to allocate that variable. In the lambda to catch if the event is called more than once. (I thought there might be race between disconnecting the event listener and another frame being pushed, although I haven't seen that in practice).
> 
> Is there only one final copy of the lambda? Or are you saying that multiple independent lambdas are stored?
> Because if the copies are only temporary, I don't see how that is an issue.

TEST(MediaEventSource, Mutable)
{
  RefPtr<TaskQueue> queue = new TaskQueue(
    GetMediaThreadPool(MediaThreadType::PLAYBACK));

  MediaEventProducer<int> source;

  bool invoked = false;
  MediaEventListener listener = source.Connect(queue,
    [invoked] (int i) mutable {
    printf("invoked=%d(%p), i=%d\n", invoked, &invoked, i);
    MOZ_ASSERT(!invoked);
    invoked = true;
  });

  source.Notify(3);
  source.Notify(3);

  queue->BeginShutdown();
  queue->AwaitShutdownAndIdle();
  listener.Disconnect();
}

output:
invoked=0(0x7fdf21057bd0), i=3
invoked=0(0x7fdf21057c30), i=3

Notify() is called twice without failing the assertion.
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review77964

> There is no way to cancel a job running on another thread perfectly. You still have a chance to get a frame produced by the blank decoder even after ResetDecode() is called.

I think the correct way to handle this is to implement a NullDecoder or DummyDecoder to produce testable invalid frames which can be checked. The resume from suspend logic can then wait until the first valid frame is received.

> TEST(MediaEventSource, Mutable)
> {
>   RefPtr<TaskQueue> queue = new TaskQueue(
>     GetMediaThreadPool(MediaThreadType::PLAYBACK));
> 
>   MediaEventProducer<int> source;
> 
>   bool invoked = false;
>   MediaEventListener listener = source.Connect(queue,
>     [invoked] (int i) mutable {
>     printf("invoked=%d(%p), i=%d\n", invoked, &invoked, i);
>     MOZ_ASSERT(!invoked);
>     invoked = true;
>   });
> 
>   source.Notify(3);
>   source.Notify(3);
> 
>   queue->BeginShutdown();
>   queue->AwaitShutdownAndIdle();
>   listener.Disconnect();
> }
> 
> output:
> invoked=0(0x7fdf21057bd0), i=3
> invoked=0(0x7fdf21057c30), i=3
> 
> Notify() is called twice without failing the assertion.

:-(
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review77964

> :-(

Should the lambda be copied when the callback is invoked? I guess it's just a sad fact of life.
Sorry if diffs for patchs P7 and P8 get messed up. I removed old patch P7 because it disappeared after refactoring to follow JW's suggestions.
Tested locally on Linux x64 Debug. Try run happening, here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a40f27c07ba
Comment on attachment 8791477 [details]
Bug 1295921 - P3: Test element becomes tainted by DrawImage.

https://reviewboard.mozilla.org/r/78908/#review80256
Attachment #8791477 - Flags: review?(bugs) → review+
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review80570

::: dom/media/platforms/moz.build:9
(Diff revision 4)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  EXPORTS += [
>      'agnostic/AgnosticDecoderModule.h',
> +    'agnostic/DummyMediaDecoder.h',

Please call it 'DummyMediaDataDecoder.h'.

::: dom/media/platforms/moz.build:24
(Diff revision 4)
>  ]
>  
>  UNIFIED_SOURCES += [
>      'agnostic/AgnosticDecoderModule.cpp',
>      'agnostic/BlankDecoderModule.cpp',
> +    'agnostic/DummyMediaDecoder.cpp',

Ditto.
Attachment #8791481 - Flags: review?(jwwang) → review-
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review80572
Attachment #8791481 - Flags: review- → review+
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review80574

::: dom/media/platforms/agnostic/DummyMediaDecoder.h:46
(Diff revision 4)
> +
> +private:
> +  void OutputFrame(MediaData* aData);
> +
> +private:
> +  nsAutoPtr<DummyDataCreator> mCreator;

Prefer UniquePtr.
Comment on attachment 8795617 [details]
Bug 1295921 - P9: NullDecoderModule returns null VideoData.

https://reviewboard.mozilla.org/r/81608/#review80576

::: dom/media/platforms/agnostic/NullDecoderModule.cpp:38
(Diff revision 2)
> +public:
> +
> +  // Decode thread.
> +  already_AddRefed<MediaDataDecoder>
> +  CreateVideoDecoder(const CreateDecoderParams& aParams) override {
> +    NullVideoDataCreator* creator = new NullVideoDataCreator();

Prefer UniquePtr over raw pointers.

::: dom/media/platforms/agnostic/NullDecoderModule.cpp:61
(Diff revision 2)
> +  }
> +
> +  ConversionRequired
> +  DecoderNeedsConversion(const TrackInfo& aConfig) const override
> +  {
> +    if (aConfig.IsVideo() && MP4Decoder::IsH264(aConfig.mMimeType)) {

Do we really need conversion for such a 'null' decoder?
Attachment #8795617 - Flags: review?(jwwang) → review+
Comment on attachment 8791476 [details]
Bug 1295921 - P2: Mark element tainted when DrawImage is used.

https://reviewboard.mozilla.org/r/78906/#review80580

::: dom/html/HTMLMediaElement.h:620
(Diff revision 2)
> +  // This is intended for cases where decoding has been suspended and
> +  // it arises that JS wants a frame to use in, eg.,
> +  // nsLayoutUtils::SurfaceFromElement() via drawImage().
> +  // *** This function will block the main thread if video decode is
> +  //     suspended!!! ***
> +  layers::Image* MarkSuspendTaintAndGetCurrentImage();

I would still prefer to not have 'Taint' in this name (just GetCurrentImage()), so that people reading layout code don't need to understand this concept. I feel that blocking is implied, and tainting is an internal concept that nsLayoutUtils doesn't need to care about (much).

The comment can still explain about the blocking and disabling of decode suspending.
Attachment #8791476 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8795619 [details]
Bug 1295921 - PB: Blocking implementation of ImageContainer AutoLockImage.

https://reviewboard.mozilla.org/r/81612/#review80582

::: gfx/layers/ImageContainer.cpp:365
(Diff revision 2)
>  
> +void
> +ImageContainer::GetCurrentImagesBlocking(nsTArray<OwningImage>* aImages)
> +{
> +  ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +  while (mCurrentImages.IsEmpty()) {

What happens if a valid image never arrives (like if the decoder hits an error and abandons decoding)?
Comment on attachment 8795619 [details]
Bug 1295921 - PB: Blocking implementation of ImageContainer AutoLockImage.

https://reviewboard.mozilla.org/r/81612/#review80584
Attachment #8795619 - Flags: review?(matt.woodrow) → review-
Comment on attachment 8795618 [details]
Bug 1295921 - PA: Return blank decoder to being green.

https://reviewboard.mozilla.org/r/81610/#review80586

Not familiar with gfx code. Please ask someone else for review. Btw,  what does "Return blank decoder to being green" mean?
Attachment #8795618 - Flags: review?(jwwang)
Comment on attachment 8795620 [details]
Bug 1295921 - PB: Implement dispatching of pending main thread MediaDecoder tasks.

https://reviewboard.mozilla.org/r/81614/#review80600

::: dom/media/MediaDecoder.cpp:1295
(Diff revision 2)
>    MOZ_ASSERT(!IsShutdown());
>    mOwner->SeekStarted();
>  }
>  
>  void
> +MediaDecoder::DispatchPendingMainThreadTasks()

Just call it 'DispatchPendingTasks' which is not about main thread tasks, but about tasks targeted for the TaskQueue of MDSM.

::: dom/media/MediaDecoder.cpp:1298
(Diff revision 2)
>  
>  void
> +MediaDecoder::DispatchPendingMainThreadTasks()
> +{
> +  AbstractThread* mainThread = AbstractThread::MainThread();
> +  mainThread->TailDispatcher().DrainDirectTasks();

This statement should be moved into MDSM::DispatchPendingMainThreadTasks().
Attachment #8795620 - Flags: review?(jwwang) → review+
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

https://reviewboard.mozilla.org/r/81616/#review80606

::: dom/html/HTMLMediaElement.cpp:834
(Diff revision 2)
>    // Mark the decoder owned by the element as tained so that the video decode
> -  // isn't suspended.
> +  // won't be suspended. If the decoder is already in suspended state, this will
> +  // resume decoding.
>    mHasSuspendTaint = true;
>    if (mDecoder) {
>      mDecoder->SetSuspendTaint(true);

One idea:
If we dispatch a task immediately in SetSuspendTaint() to tell MDSM to change its tained status without using canonical/mirror, we won't need DispatchPendingMainThreadTasks() below.
Attachment #8795621 - Flags: review?(jwwang) → review+
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review77964

> To flush out any pending frames from the blank decoder. Ideally, we would never add the blank frames to the decoder and expire frames in the past.

Changed how patch was implemented. Closing this issue.
Comment on attachment 8791481 [details]
Bug 1295921 - P8: Extract BlankMediaDataDecoder so it can be shared.

https://reviewboard.mozilla.org/r/78916/#review80574

> Prefer UniquePtr.

Roger. Switched to UniquePtr and changed constructor to take UniquePtr<T>&& to signify it's a sink for the parameter and going to take ownership.
Comment on attachment 8795617 [details]
Bug 1295921 - P9: NullDecoderModule returns null VideoData.

https://reviewboard.mozilla.org/r/81608/#review80906

::: dom/media/platforms/agnostic/NullDecoderModule.cpp:61
(Diff revision 2)
> +  }
> +
> +  ConversionRequired
> +  DecoderNeedsConversion(const TrackInfo& aConfig) const override
> +  {
> +    if (aConfig.IsVideo() && MP4Decoder::IsH264(aConfig.mMimeType)) {

I'm not sure. Will ask @jya.
Comment on attachment 8791476 [details]
Bug 1295921 - P2: Mark element tainted when DrawImage is used.

https://reviewboard.mozilla.org/r/78906/#review80580

> I would still prefer to not have 'Taint' in this name (just GetCurrentImage()), so that people reading layout code don't need to understand this concept. I feel that blocking is implied, and tainting is an internal concept that nsLayoutUtils doesn't need to care about (much).
> 
> The comment can still explain about the blocking and disabling of decode suspending.

Seems totally reasonable. Done.
Comment on attachment 8795618 [details]
Bug 1295921 - PA: Return blank decoder to being green.

https://reviewboard.mozilla.org/r/81610/#review80586

I think Dan intends to revert this patch: https://reviewboard.mozilla.org/r/64268/diff/6#index_header , which makes the blank video creator use less memory.
(In reply to Tzuhao Kuo [:kaku] (PTO until 10/2) from comment #86)
> Comment on attachment 8795618 [details]
> Bug 1295921 - PA: Return blank decoder to being green.
> 
> https://reviewboard.mozilla.org/r/81610/#review80586
> 
> I think Dan intends to revert this patch:
> https://reviewboard.mozilla.org/r/64268/diff/6#index_header , which makes
> the blank video creator use less memory.

Yes.
Comment on attachment 8795619 [details]
Bug 1295921 - PB: Blocking implementation of ImageContainer AutoLockImage.

https://reviewboard.mozilla.org/r/81612/#review80584

After much thinking about this and balancing JW and Matt's review comments, I decided that the MDSM is the piece of the architecture that has the best knowledge of decoding and errors. I don't want to plumb all the error handling logic through MediaDecoderStateMachine, VideoSink, VideoQueue and ImageContainer, so I'm going to go back to a design that introduces a temporary Monitor into a MediaDecoder function that is called just before accessing ImageContainer's image.
Comment on attachment 8795619 [details]
Bug 1295921 - PB: Blocking implementation of ImageContainer AutoLockImage.

https://reviewboard.mozilla.org/r/81612/#review83640

::: gfx/layers/ImageContainer.cpp:365
(Diff revision 2)
>  
> +void
> +ImageContainer::GetCurrentImagesBlocking(nsTArray<OwningImage>* aImages)
> +{
> +  ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +  while (mCurrentImages.IsEmpty()) {

I removed blocking from ImageContainer because it doesn't know if a frame is never going to arrive because of an error in the MDSM.

I've moved the blocking logic into the MDSM, which knows about success/failure.
Attachment #8795619 - Attachment is obsolete: true
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

Asking for review again from JW because of large changes to this patch.
Attachment #8795621 - Flags: review+ → review?(jwwang)
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

https://reviewboard.mozilla.org/r/81616/#review83702

::: dom/html/HTMLMediaElement.cpp:827
(Diff revision 3)
> +  mHasSuspendTaint = true;
> +  if (mDecoder) {
> +    mDecoder->WaitOnNextRenderedVideoFrame();
> +  }
> +
> +  // If element has a decoder, block and get next decoded image, otherwise

This comment should  go above
|mDecoder->WaitOnNextRenderedVideoFrame();|

::: dom/media/MediaDecoderStateMachine.cpp:2164
(Diff revision 3)
> +    // If wait for frame state is set, need to wait for the next frame
> +    // to be rendered and then signal the monitor to unblock the
> +    // waiting main thread.
> +    if (mWaitForFrameState) {
> +      mRenderVideoListener =
> +        mMediaSink->RenderEvent().Connect(mTaskQueue, [=](){

I don't think we need this 'render event' from the media sink. All we need is to call MarkWaitForFrameDone() when video-only seek is done (no matter it is a success or failure).

::: dom/media/MediaDecoderStateMachine.cpp:2201
(Diff revision 3)
> +    auto reportRecoveryTelemetry = [=](){
> +      ReportRecoveryTelemetry(start, info, hw);
> +    };
> +
>      RefPtr<MediaDecoder::SeekPromise> p = seekJob.mPromise.Ensure(__func__);
> -    p->Then(AbstractThread::MainThread(), __func__,
> +    p->Then(mTaskQueue, __func__,

Reporting telemetry and calling MarkWaitForFrameDone() should be orthogonal. You can do:

p->Then(AbstractThread::MainThread(), ...); // report telemetry

p->Then(mTaskQueue, ...); // call MarkWaitForFrameDone()

Btw, you need to change MediaDecoder::SeekPromise to be non-exclusive. Or you can use promise chaining instead.
Attachment #8795621 - Flags: review?(jwwang) → review-
Comment on attachment 8795618 [details]
Bug 1295921 - PA: Return blank decoder to being green.

https://reviewboard.mozilla.org/r/81610/#review83706

gfx guy should review this change.
Attachment #8795618 - Flags: review?(jwwang)
Target Milestone: --- → mozilla52
Blocks: 1309492
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

https://reviewboard.mozilla.org/r/81616/#review83702

> I don't think we need this 'render event' from the media sink. All we need is to call MarkWaitForFrameDone() when video-only seek is done (no matter it is a success or failure).

I added 'render event' because I really want to know when the frame has been added to ImageContainer.  From all the distribution of logic, it was clear to me that the frame is added before seek promise is resolved.

Is it certain that VideoSink::TryRenderVideoFrames() is called before the SeekPromise resolved?

> Reporting telemetry and calling MarkWaitForFrameDone() should be orthogonal. You can do:
> 
> p->Then(AbstractThread::MainThread(), ...); // report telemetry
> 
> p->Then(mTaskQueue, ...); // call MarkWaitForFrameDone()
> 
> Btw, you need to change MediaDecoder::SeekPromise to be non-exclusive. Or you can use promise chaining instead.

I tried p->Then->Then and it doesn't compile. I went with version I did because I didn't want to change SeekPromise to be non-exclusive. If you say that making SeekPromise non-exclusive is OK, then I'll do that.
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

https://reviewboard.mozilla.org/r/81616/#review83702

> I added 'render event' because I really want to know when the frame has been added to ImageContainer.  From all the distribution of logic, it was clear to me that the frame is added before seek promise is resolved.
> 
> Is it certain that VideoSink::TryRenderVideoFrames() is called before the SeekPromise resolved?

http://searchfox.org/mozilla-central/rev/2142de26c16c05f23e543be4fa1a651c4d29604e/dom/media/MediaDecoderStateMachine.cpp#907
mMediaSink->Redraw is called in SeekCompleted() which guarantees there is something in the image container when the seek promise is resolved.

> I tried p->Then->Then and it doesn't compile. I went with version I did because I didn't want to change SeekPromise to be non-exclusive. If you say that making SeekPromise non-exclusive is OK, then I'll do that.

If you want promise chaining:
p->Then(...)->CompletionPromise()->Then(...);

I am fine with SeekPromise being non-exclusive.
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

https://reviewboard.mozilla.org/r/81616/#review83702

> http://searchfox.org/mozilla-central/rev/2142de26c16c05f23e543be4fa1a651c4d29604e/dom/media/MediaDecoderStateMachine.cpp#907
> mMediaSink->Redraw is called in SeekCompleted() which guarantees there is something in the image container when the seek promise is resolved.

Excellent. I was uncomfortable with adding the RenderEvent!
This is currently looking pretty good on try. There were some flaky timeouts in a test I added that used 'timeupdate' event. I've removed that for now, but maybe it makes the test less rigorous?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=118483452c079235397219dc63cb34b975b5fbdd&selectedJob=29251995
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

Remove Matt.
Attachment #8795621 - Flags: review?(matt.woodrow)
Comment on attachment 8795618 [details]
Bug 1295921 - PA: Return blank decoder to being green.

https://reviewboard.mozilla.org/r/81610/#review85396
Attachment #8795618 - Flags: review?(jwwang) → review+
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

https://reviewboard.mozilla.org/r/81616/#review85408

::: dom/media/MediaDecoder.h:396
(Diff revision 4)
>     * thread.
>     ******/
>  
> -  // Dispatch any pending tasks in the main thread tail dispatcher in order.
> -  void DispatchPendingTasks();
> +  // Cause the decoder to wait for image container to hold a valid frame. If the
> +  // image container doesn't contain any frames, WaitOnNextRenderedVideoFrame
> +  // will block the thread until a frame in added into the image container. In

'is' added

::: dom/media/MediaDecoder.cpp:1333
(Diff revision 4)
> +  mDecoderStateMachine->DispatchSetWaitForFrameState(&state);
> +  SetSuspendTaint(true);
> +
> +  // Decoder is marked as tainted, but it might be suspended already. The
> +  // called AutoLockImage will cause the MainThread to block until the frame
> +  // is decoded.  To achieve this, need into a stable state where any pending

need 'to go' into a stable state...

::: dom/media/MediaDecoder.cpp:1335
(Diff revision 4)
> +
> +  // Decoder is marked as tainted, but it might be suspended already. The
> +  // called AutoLockImage will cause the MainThread to block until the frame
> +  // is decoded.  To achieve this, need into a stable state where any pending
> +  // variable state updates has been dispatched to MDSM.  This is done by
> +  // DispatchPendingMainThreadTasks.

s/DispatchPendingMainThreadTasks/DispatchPendingTasks/.

::: dom/media/MediaDecoderStateMachine.cpp:2312
(Diff revision 4)
>        return;
>      }
>  
>      // If an existing seek is in flight don't bother creating a new
>      // one to catch up.
>      if (mState == DECODER_STATE_SEEKING || mQueuedSeek.Exists()) {

MarkWaitForFrameDone() will never be called if we return early here. And the main thread will get stuck forever.

I think need to fix bug 1310140 first where P10 switches off the blank/null decoder when seek starts. So we are guaranteed to have some valid frames when seek is done. Then we just need to call MarkWaitForFrameDone() in SeekCompleted() to notify those waiting for valid frames.
Attachment #8795621 - Flags: review?(jwwang) → review-
Depends on: 1310140
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

https://reviewboard.mozilla.org/r/81616/#review85480

Drive-by review.

::: dom/media/MediaDecoderStateMachine.h:245
(Diff revision 4)
>    }
>  
> +  // WaitForFrame holds a monitor protected flag used to signal to the
> +  // main thread that it can stop blocking.
> +  struct WaitForFrame {
> +    explicit WaitForFrame(const char* desc) :

s/desc/aDesc/.
Blocks: 1309494
Attached patch debug-1.patch — — Splinter Review 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        debug-2.patch
        — 
          — Splinter Review
      

    


  

    
    

    
  
@Dan, 

While I was debugging bug 1309492 comment 5, I found that the "test_background_video_drawimage_with_suspended_video.html" is not doing what we're expected. It always succeeds no matter what decoder module is used, however, it should fails if we're using blank decoder module. If the blank decoder module is used, it should then draw a green frame onto a canvas and then all the pixels have the same color. You can apply the attachment 8803300 [details] [diff] [review] to verify.

The problem is that we might have mistaken the meaning of the testVideoResumesWhenShown() helper function (http://searchfox.org/mozilla-central/rev/2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/dom/media/test/background_video.js#77). The testVideoResumesWhenShown() hides a video element and then waits for the "mozexitvideosuspend" event, however, the "mozexitvideosuspend" event only indicates that the MFR is going to switch the decoder but not yet done.

In that case, in the "test_background_video_drawimage_with_suspended_video.html", while we're drawing a video onto a canvas, the video's video decoder is under switching to a null decoder but not done yet, so the video still has valid frames in its ImageContainer and then the video always passes the check |if (GetImageContainer()->HasCurrentImage())| at the beginning of the MediaDecoder::WaitOnNextRenderedVideoFrame() (https://hg.mozilla.org/try/rev/25be32dc4a28c5ec1ed930525eaff040ea2ad346#l2.16). This way, we never really get to lock the monitor and wait to be notified. 

What is interesting is that I use attachment 8803301 [details] [diff] [review] to simulate "switching decoder DONE" event (use a for-loop to wait for a while...) and then the monitor will never be notified, which is exactly the same situation which is described at bug 1309492 comment 5. 

Please take a look at this issue, I will also keep working on it.
Flags: needinfo?(dglastonbury)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

https://reviewboard.mozilla.org/r/81616/#review85408

> MarkWaitForFrameDone() will never be called if we return early here. And the main thread will get stuck forever.
> 
> I think need to fix bug 1310140 first where P10 switches off the blank/null decoder when seek starts. So we are guaranteed to have some valid frames when seek is done. Then we just need to call MarkWaitForFrameDone() in SeekCompleted() to notify those waiting for valid frames.

I'll rebase onto bug 1310140. But the issue is I need to block on the main thread ideally with out querying a flag owned by the ThreadPool side of the MDSM, which could be racy.

I'm thinking I could put an RAII helper into VideoDecodeSuspendChanged() that will call MarkWaitForFrameDone(), unless the "trigger is disarmed" when code gets setting up recovery seek job.

    
    

    
  
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

https://reviewboard.mozilla.org/r/81616/#review85408

> I'll rebase onto bug 1310140. But the issue is I need to block on the main thread ideally with out querying a flag owned by the ThreadPool side of the MDSM, which could be racy.
> 
> I'm thinking I could put an RAII helper into VideoDecodeSuspendChanged() that will call MarkWaitForFrameDone(), unless the "trigger is disarmed" when code gets setting up recovery seek job.

OK. I've finished rebasing on to 1310140 and have checked that all code paths should call MarkWaitForFrameDone().

    
    

    
  
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

https://reviewboard.mozilla.org/r/81616/#review86854

::: dom/media/MediaDecoder.h:397
(Diff revision 5)
>     ******/
>  
> -  // Dispatch any pending tasks in the main thread tail dispatcher in order.
> -  void DispatchPendingTasks();
> +  // Cause the decoder to wait for image container to hold a valid frame. If the
> +  // image container doesn't contain any frames, WaitOnNextRenderedVideoFrame
> +  // will block the thread until a frame is added into the image container. In
> +  // the case that video decode is suspended and a seek failure occurs, the

s/and/or/

        Attachment #8795621 -
        Flags: review?(jwwang) → review+

    
    

    
  
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

https://reviewboard.mozilla.org/r/81616/#review86854

> s/and/or/

Actually this should be and here. I'm trying to say, WaitOnNextRenderedVideoFrame() will block until a frame is added to the container, so calling GetImages on ImageContainer should contain at least one frame. But in the case of an error, WaitOnNextRenderedVideoFrame() will return and the ImageContainer will not contain a frame.

Maybe I should just make this the comment.

    
    

    
  
(In reply to Tzuhao Kuo [:kaku] from comment #127)
> @Dan, 
> 
> While I was debugging bug 1309492 comment 5, I found that the
> "test_background_video_drawimage_with_suspended_video.html" is not doing
> what we're expected. It always succeeds no matter what decoder module is
> used, however, it should fails if we're using blank decoder module. If the
> blank decoder module is used, it should then draw a green frame onto a
> canvas and then all the pixels have the same color. You can apply the
> attachment 8803300 [details] [diff] [review] to verify.
> 
> The problem is that we might have mistaken the meaning of the
> testVideoResumesWhenShown() helper function
> (http://searchfox.org/mozilla-central/rev/
> 2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/dom/media/test/background_video.
> js#77). The testVideoResumesWhenShown() hides a video element and then waits
> for the "mozexitvideosuspend" event, however, the "mozexitvideosuspend"
> event only indicates that the MFR is going to switch the decoder but not yet
> done.
> 
> In that case, in the
> "test_background_video_drawimage_with_suspended_video.html", while we're
> drawing a video onto a canvas, the video's video decoder is under switching
> to a null decoder but not done yet, so the video still has valid frames in
> its ImageContainer and then the video always passes the check |if
> (GetImageContainer()->HasCurrentImage())| at the beginning of the
> MediaDecoder::WaitOnNextRenderedVideoFrame()
> (https://hg.mozilla.org/try/rev/25be32dc4a28c5ec1ed930525eaff040ea2ad346#l2.
> 16). This way, we never really get to lock the monitor and wait to be
> notified. 
> 
> What is interesting is that I use attachment 8803301 [details] [diff] [review]
> [review] to simulate "switching decoder DONE" event (use a for-loop to wait
> for a while...) and then the monitor will never be notified, which is
> exactly the same situation which is described at bug 1309492 comment 5. 
> 
> Please take a look at this issue, I will also keep working on it.

Kaku, Thanks for the analysis. I'll take a look to see if I can make sense of what's wrong. The intention of this test is that when calling drawImage() on a suspended video, the result should be video frame and not all white or all green, etc.

The behaviour of resuming really relies on the NullDecoder not putting any frames into the image container while decoding is suspended. (As you describe |if (GetImageContainer()->HasCurrentImage())|)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Matt, 

Could you help us out with the CanvasRenderingContext and nsLayoutUtil changes in Patch D?

Kaku discovered an issue with the blocking main thread in drawImage() that I implemented. With the webm demuxer, it would hit the media cache and main thread, which is blocked and thus we'd dead lock. Kaku changed the monitor out for spinning the event loop, waiting for the a task from the MDSM to set a variable.

This fixed the media cache issue, but introduced a problem with the borrowed DrawTarget being released by the task scheduled from ScheduleStableStateCallback(). I tried a number of ways to keep the DrawTarget alive, but kept hitting asserts. The method in PD was implemented by Kaku and works reliably but I'm fearful we're broken some constraints by spinning the event loop.

    
    

    
  
I spoke to :jya and :cpearce and they weren't happy with spinning the event loop because once you do that all assumptions are off-the-table. :-(

    
    

    
  
Spinning the event loop from within code run from JS is pretty terrifying, I'd have to agree with them there.

    
    

    
  
Comment on attachment 8808076 [details]
Bug 1295921 - PD: Keep last keyframe in MFR.

https://reviewboard.mozilla.org/r/91002/#review91016

I don't think it's going to be safe to do this, sorry.

        Attachment #8808076 -
        Flags: review?(matt.woodrow) → review-

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8808076 -
        Flags: review?(matt.woodrow) → feedback?(jyavenard)

    
    

    
  
Comment on attachment 8808076 [details]
Bug 1295921 - PD: Keep last keyframe in MFR.

https://reviewboard.mozilla.org/r/91002/#review92212

::: dom/media/MediaDecoderReader.h:299
(Diff revision 2)
>    // queued in the original video decoder.
>    virtual void SetVideoBlankDecode(bool aIsBlankDecode) {}
>  
> +  // Inform the reader that the main thread is blocked and calls/tasks
> +  // dispatched to main thread shouldn't be used.
> +  virtual void SetMainThreadBlocked(bool) {}

Speaking to :jya offline, he said that this feature should introduce a new seek type. Adding a new seek type would remove the need to plumb the "main thread is blocked" through to the MFR.

::: dom/media/MediaFormatReader.h:297
(Diff revision 2)
> +    Seek(media::TimeUnit aTime, SuccessMethod aSuccess, FailureMethod aFailure)
> +    {
> +      if (mType == MediaData::VIDEO_DATA && mOwner->mMainThreadBlocked) {
> +        MOZ_ASSERT(mQueuedSamples.IsEmpty());
> +
> +        RefPtr<MediaRawData> temp = mLastDemuxedKeyframe->Clone();

If the seeking video and the main thread is blocked, then clone the compressed data for the last keyframe and feed it into mQueuedSamples for processing and schedule update to start decoding.

When seek is completed, MDSM will RequestVideoData that will cause the MDSM to get the decoded frame.

Set the time stamp to the time of the requested seek point, since this is what resume from dormant mode does in AccurateSeekTask.

::: dom/media/MediaFormatReader.h:300
(Diff revision 2)
> +        MOZ_ASSERT(mQueuedSamples.IsEmpty());
> +
> +        RefPtr<MediaRawData> temp = mLastDemuxedKeyframe->Clone();
> +        temp->mTime = aTime.ToMicroseconds();
> +        mQueuedSamples.AppendElement(temp.forget());
> +        mOwner->ScheduleUpdate(TrackInfo::kVideoTrack);

:jya says that need to drain the decoder to get the frame back. What's currently here works, but doesn't drain.

    
  
Depends on: 1319992

        Attachment #8808076 -
        Flags: feedback?(jyavenard)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Kaku,

Thanks for taking on this bug and helping to get these patches landed. These patches need Bug 1319992 to land to work correctly. Looks like those patches were backed out, so you'll have to apply the latest before testing these.

I wrote a large comment on Patch D to explain the last approach to handling recovery under drawImage(), I'm hoping this is enough to understand the logic. Once I removed all my logging and debugging changes, it turned out to not be too much code.

I added a new state to handle the recovery seeking to the MDSM, called BlockingResumeVideoState. Mostly it's copied from AccurateSeekingState, so it's very likely that BlockingResumeVideoState can be removed and the logic integrated with AccurateSeekingState. Check with :jwwang.

Also, the new SeekTarget mode, PrevFrame, isn't the best name. Feel free to change it to something better.
Assignee: dglastonbury → kaku
Flags: needinfo?(kaku)

    
    

    
  
Comment on attachment 8808076 [details]
Bug 1295921 - PD: Keep last keyframe in MFR.

https://reviewboard.mozilla.org/r/91002/#review99650

::: dom/media/MediaDecoderStateMachine.cpp:90
(Diff revision 3)
>  #define SLOG(x, ...) MOZ_LOG(gMediaDecoderLog, LogLevel::Debug, (SFMT(x, ##__VA_ARGS__)))
>  #define SWARN(x, ...) NS_WARNING(nsPrintfCString(SFMT(x, ##__VA_ARGS__)).get())
>  #define SDUMP(x, ...) NS_DebugBreak(NS_DEBUG_WARNING, nsPrintfCString(SFMT(x, ##__VA_ARGS__)).get(), nullptr, nullptr, -1)
>  #define SSAMPLELOG(x, ...) MOZ_LOG(gMediaSampleLog, LogLevel::Debug, (SFMT(x, ##__VA_ARGS__)))
>  
> +LazyLogModule gDecoderSuspendLog("DecodeSuspend");

Do we really need a new log module? The existing DECODER_LOG should do the job for it doesn't print as many messags as SAMPLE_LOG.

::: dom/media/MediaDecoderStateMachine.cpp:1478
(Diff revision 3)
> +    MOZ_ASSERT(!mDoneVideoSeeking, "Seek shouldn't be finished");
> +
> +    RefPtr<MediaData> audio(aAudio);
> +    MOZ_ASSERT(audio);
> +
> +    // The MDSM::mDecodedAudioEndTime will be updated once the whole SeekTask is

Remove this comment and log which are already included in MDSM::OnAudioDecoded().

::: dom/media/MediaDecoderStateMachine.cpp:1486
(Diff revision 3)
> +    SSAMPLELOG("HandleAudioDecoded [%lld,%lld]", audio->mTime, audio->GetEndTime());
> +
> +    // Video-only seek doesn't reset audio decoder. There might be pending audio
> +    // requests when AccurateSeekTask::Seek() begins. We will just store the data
> +    // without checking |mDiscontinuity| or calling DropAudioUpToSeekTarget().
> +    mSeekedAudioData = audio.forget();

Just do
mSeekedAudioData = aAudio;
without declaring a local variable.

Also fix the comment for there is no mDiscontinuity anymore nor DropAudioUpToSeekTarget in this class.

::: dom/media/MediaDecoderStateMachine.cpp:1496
(Diff revision 3)
> +    MOZ_ASSERT(!mDoneVideoSeeking, "Seek shouldn't be finished");
> +
> +    RefPtr<MediaData> video(aVideo);
> +    MOZ_ASSERT(video);
> +
> +    // The MDSM::mDecodedVideoEndTime will be updated once the whole SeekTask is

Ditto.

::: dom/media/MediaDecoderStateMachine.cpp:1502
(Diff revision 3)
> +    // resolved.
> +
> +    SSAMPLELOG("HandleVideoDecoded [%lld,%lld]", video->mTime, video->GetEndTime());
> +
> +    // Non-precise seek. We can stop the seek at the first sample.
> +    mSeekedVideoData = video;

mSeekedVideoData = aVideo.

::: dom/media/MediaDecoderStateMachine.cpp:1512
(Diff revision 3)
> +
> +  void HandleNotDecoded(MediaData::Type aType, const MediaResult& aError) override
> +  {
> +    MOZ_ASSERT(!mDoneVideoSeeking, "Seek shouldn't be finished");
> +
> +    SSAMPLELOG("OnNotDecoded type=%d reason=%u", aType, aError.Code());

Remove this log.

::: dom/media/MediaDecoderStateMachine.cpp:1527
(Diff revision 3)
> +      Reader()->WaitForData(aType);
> +      return;
> +    }
> +
> +    if (aError == NS_ERROR_DOM_MEDIA_CANCELED &&
> +        aType == MediaData::VIDEO_DATA) {

aType is guaranteed to be MediaData::VIDEO_DATA for the return at #1515.

::: dom/media/MediaDecoderStateMachine.cpp:1533
(Diff revision 3)
> +      RequestVideoData();
> +      return;
> +    }
> +
> +    if (aError == NS_ERROR_DOM_MEDIA_END_OF_STREAM) {
> +      if (aType == MediaData::AUDIO_DATA) {

See #1515.

::: dom/media/MediaDecoderStateMachine.cpp:1569
(Diff revision 3)
> +  }
> +
> +private:
> +  void CreateSeekTask() override
> +  {
> +    mDoneVideoSeeking = false;

mDoneVideoSeeking is already initialized to false.

::: dom/media/MediaDecoderStateMachine.cpp:1598
(Diff revision 3)
> +
> +  void OnSeekResolved(media::TimeUnit)
> +  {
> +    mSeekRequest.Complete();
> +
> +    if (!mDoneVideoSeeking) {

mDoneVideoSeeking should be false here, right?

::: dom/media/MediaDecoderStateMachine.cpp:1630
(Diff revision 3)
> +
> +  void OnSeekTaskResolved()
> +  {
> +    if (mSeekedAudioData) {
> +      mMaster->Push(mSeekedAudioData);
> +      mMaster->mDecodedAudioEndTime = std::max(

mDecodedAudioEndTime is already updated in MDSM::OnAudioDecoded().

::: dom/media/MediaDecoderStateMachine.cpp:1636
(Diff revision 3)
> +        mSeekedAudioData->GetEndTime(), mMaster->mDecodedAudioEndTime);
> +    }
> +
> +    if (mSeekedVideoData) {
> +      mMaster->Push(mSeekedVideoData);
> +      mMaster->mDecodedVideoEndTime = std::max(

Ditto.

::: dom/media/MediaDecoderStateMachine.cpp:1672
(Diff revision 3)
> +  MozPromiseRequestHolder<MediaDecoderReader::SeekPromise> mSeekRequest;
> +
> +  /*
> +   * Internal state.
> +   */
> +  media::TimeUnit mCurrentTimeBeforeSeek;

This field is for fast-seeking only.

    
    

    
  
> Because the decoder has been flushed, the MDSM starts a second seek to
> the current time stamp to recover the video stream.

Why won't the 2nd seek cause deadlock as long as the main thread is still blocking?

    
    

    
  
Comment on attachment 8808076 [details]
Bug 1295921 - PD: Keep last keyframe in MFR.

https://reviewboard.mozilla.org/r/91002/#review99720

LGTM for the MFR changes...

::: dom/media/MediaFormatReader.cpp:1397
(Diff revision 3)
>  {
>    MOZ_ASSERT(OnTaskQueue());
>    LOGV("Decoder has requested more %s data", TrackTypeToStr(aTrack));
>    auto& decoder = GetDecoderData(aTrack);
>    decoder.mDecodePending = false;
> +  // If forcing the last keyframe and the frame is pending decode, drain

the comment imply that we are in pending decode. But the line above set that to false.

::: dom/media/MediaFormatReader.cpp:1401
(Diff revision 3)
>    decoder.mDecodePending = false;
> +  // If forcing the last keyframe and the frame is pending decode, drain
> +  // the decoder.
> +  if (decoder.mForceLastKeyframe) {
> +    LOGV("ForceLastKeyframe set. Clearing and forcing need draining.");
> +    decoder.mForceLastKeyframe = false;

I would have preferred this to be handled in Update(), but I can't see an easy way to do that without adding heaps of confusing logic.

So I guess that will do.

::: dom/media/MediaFormatReader.cpp:1708
(Diff revision 3)
>      if (aTrack == TrackInfo::kVideoTrack) {
>        aA.mStats.mParsedFrames++;
>      }
>  
> +    // If the sample is a keyframe, keep a reference to it.
> +    if (aTrack == TrackInfo::kVideoTrack && sample->mKeyframe) {

Just above, we already have a test to check if it's the video track.

Please move that in that block.

        Attachment #8808076 -
        Flags: review?(jyavenard) → review+

    
    

    
  
Comment on attachment 8795621 [details]
Bug 1295921 - PC: Implement blocking image get for SurfaceFromElement.

https://reviewboard.mozilla.org/r/81616/#review102758

::: dom/media/MediaDecoder.cpp:1196
(Diff revision 9)
> +  WaitForFrame state{ "WaitOnNextRenderedVideoFrame" };
> +
> +  // Dispatch the variable setting before changing the suspend taint
> +  // that'll trigger the decoder to start up.
> +  mDecoderStateMachine->DispatchSetWaitForFrameState(&state);
> +  SetSuspendTaint(true);

There is a problem here.

Now, drawing an already-suspended video element onto a canvas element works well for the first time, but from the 2nd time on, the main thread still gets blocked.

The root cause is that we rely on mirroring the MediaDecoder::mHasSuspendTaint changes to trigger the MDSM::SuspendTaintChanged() which is in charge of notifying the main thread. However, the Canonical does NOT dispatch to its Mirror if the new assigned value equals the old value. (http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/xpcom/threads/StateMirroring.h#172-174)

To solve this problem, we should add the following check at the beginning of this function:

```
  if (mHasSuspendTaint) {
    return;
  }
```

    
    

    
  
I have rebased these patches onto Bug 1319992, however, the current code does not work on Windwos because the WMF decoder module needs to initialize the WMFVideoMFTManager on the main thread. (http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/dom/media/platforms/wmf/WMFVideoMFTManager.cpp#408-415)

Try to figure out a way...
Flags: needinfo?(kaku)

    
    

    
  
(In reply to Tzuhao Kuo [:kaku] from comment #198)
> I have rebased these patches onto Bug 1319992, however, the current code
> does not work on Windwos because the WMF decoder module needs to initialize
> the WMFVideoMFTManager on the main thread.
> (http://searchfox.org/mozilla-central/rev/
> 0f254a30d684796bcc8b6e2a102a0095d25842bb/dom/media/platforms/wmf/
> WMFVideoMFTManager.cpp#408-415)

:mattwoodrow,
Is there any chance that we can refactor the initializaion of DXVA2Manager to be off-main-thread?

I guess that you know what this bug is doing but still a brief intro:
Dan and I are working on the suspend-video-decoder feature. While a video element becomes invisible, we switch its video decoder into a light wight one. This bug tries to handle a case that drawing a already-suspended-video-element onto a canvas element. We need to switch the already-suspended-video-element's light-weight video decoder back to its original one. Since the switching video decoder task is async, we block to the main thread to wait for it. So, here is the problem, on Windows, we the video element might use WMF decoder which needs to create and initialize a DXVA2Manager and the initialization task of DXVA2Manager is main-thread only, which has already been blocked.
Flags: needinfo?(matt.woodrow)

    
    

    
  
The only reason we need main-thread for DXVA2Manager is for the crash guard.

When we have the GPU process, we probably don't need the crash guard as well, so maybe we could look into getting rid of it?

Alternatively, the main thread is blocked in this crash, so we can't race and it should be possible in theory to run the crash guard.
Flags: needinfo?(matt.woodrow)

    
    

    
  
(In reply to Matt Woodrow (:mattwoodrow) from comment #200)
> The only reason we need main-thread for DXVA2Manager is for the crash guard.
> 
> When we have the GPU process, we probably don't need the crash guard as
> well, so maybe we could look into getting rid of it?
What about the FindDXVABlacklistedDLL() which uses ClearOnShutdown()? (also refer to bug 1316222 comment 3), is it also a reason why the CreateDXVAManagerEvent needs to be run on the main-thread?
Flags: needinfo?(matt.woodrow)

    
    

    
  
(In reply to Tzuhao Kuo [:kaku] from comment #201)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #200)
> > The only reason we need main-thread for DXVA2Manager is for the crash guard.
> > 
> > When we have the GPU process, we probably don't need the crash guard as
> > well, so maybe we could look into getting rid of it?
> What about the FindDXVABlacklistedDLL() which uses ClearOnShutdown()? (also
> refer to bug 1316222 comment 3), is it also a reason why the
> CreateDXVAManagerEvent needs to be run on the main-thread?

Yeah, good point. That one seems like it would be easier to fix though.
Flags: needinfo?(matt.woodrow)

    
    

    
  
Comment on attachment 8808076 [details]
Bug 1295921 - PD: Keep last keyframe in MFR.

Ask review again when the comments are addressed.

        Attachment #8808076 -
        Flags: review?(jwwang)

    
    

    
  
(In reply to Matt Woodrow (:mattwoodrow) from comment #200)
> The only reason we need main-thread for DXVA2Manager is for the crash guard.
> 
> When we have the GPU process, we probably don't need the crash guard as
> well, so maybe we could look into getting rid of it?

I would like to remove the crash guard from DXVA2Manager but it looks like that we're still using the WMF decoder in the content process (http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/dom/media/platforms/PDMFactory.cpp#381-382). 

However, looking into Bug 1314192 (which added the above two lines), do we only fallback to normal WFM PDM for audio?

If the answer if yes, then I think I can open a new bug to remove the crash guard since it's only used in the video case which is in the GPU process.
Flags: needinfo?(matt.woodrow)

    
    

    
  
Comment on attachment 8791480 [details]
Bug 1295921 - P6: Move resume from suspend into new function.

https://reviewboard.mozilla.org/r/78914/#review106928

::: dom/media/MediaDecoderStateMachine.cpp:2868
(Diff revision 9)
> -  if (mPlayState != MediaDecoder::PLAY_STATE_PLAYING) {
> +  if (mMinimizePreroll) {
> -    CancelSuspendTimer();
> -  } else if (mMinimizePreroll) {

Here should be 
```
if (mPlayState == MediaDecoder::PLAY_STATE_PLAYING && mMinimizePreroll) {
...
}
```

    
  
Depends on: 1337301

    
    

    
  
(In reply to Tzuhao Kuo [:kaku] from comment #204)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #200)
> > The only reason we need main-thread for DXVA2Manager is for the crash guard.
> > 
> > When we have the GPU process, we probably don't need the crash guard as
> > well, so maybe we could look into getting rid of it?
> 
> I would like to remove the crash guard from DXVA2Manager but it looks like
> that we're still using the WMF decoder in the content process
> (http://searchfox.org/mozilla-central/rev/
> 790b2cb423ea1ecb5746722d51633caec9bab95a/dom/media/platforms/PDMFactory.
> cpp#381-382). 
> 
> However, looking into Bug 1314192 (which added the above two lines), do we
> only fallback to normal WFM PDM for audio?
> 
> If the answer if yes, then I think I can open a new bug to remove the crash
> guard since it's only used in the video case which is in the GPU process.

Sorry for the delayed reply.

We should only be falling back to in-process decoding for audio, if either pref is disabled (gpu process, or remote decoding), or if the GPU process has been disabled by a crash.

Only the pref option would allow DXVA decoding though. If we assume that we're going to keep the prefs enabled by default, then I think we're fine to remove the crash guard.

I'll forward this on to dvander (the crash guard author) in case he has any objections.
Flags: needinfo?(matt.woodrow) → needinfo?(dvander)

    
    

    
  
(In reply to Matt Woodrow (:mattwoodrow) from comment #206)
> (In reply to Tzuhao Kuo [:kaku] from comment #204)
> > 
> > I would like to remove the crash guard from DXVA2Manager

Personally I would like to see the crash guard go away. *But* it unfortunately might still serve a purpose. The GPU process isn't enabled everywhere - notably Windows 7 pre-Platform Update. It's also not enabled for non-e10s users. And unfortunately, we don't have Telemetry on how often it's preventing a crash. (We do for the layers crash guard, but not the media one.)

So, I'd lean toward keeping it around. Alternately, we could add some Telemetry on how often it prevents a crash. If it's not pulling its weight, there's no reason to have it.
Flags: needinfo?(dvander)

    
    

    
  
We're going to ship the suspend-video-decoder feature without synchronously resuming decoder at this moment. So, I will then decompose this bug into two small parts; one is for tracking a video element as taint and never suspend it again, the other one is for synchronous decoding.

    
  
Depends on: 1345403

    
  
Depends on: 1345404

    
  
Blocks: 1346120

    
    

    
  
Once Bug 1338218 is fixed, the problem mentioned in comment 173 could be solved. But it may require some refactoring in MediaResource.
Depends on: PBg-HTTP

    
  
Summary: Don't suspend video decoder for elements as sources to drawImage() and createImageBitmap() → Don't suspend video decoder for elements as sources to drawImage()

    
  
No longer blocks: Phase_1_Shutdown_Decoder

    
  
No longer blocks: 1309492

    
  
No longer depends on: PBg-HTTP

    
  
No longer blocks: 1309494

    
  
No longer blocks: 1346120

    
  
Blocks: Shutdown-Decoder

    
  
No longer blocks: 1276556

    
    

    
  
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.


Assignee: kakukogou → nobody
Status: ASSIGNED → NEW

    
  
Severity: normal → S3

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: