1309492 - If a video element's video is suspended, make it draw black frames.

Status: RESOLVED WONTFIX

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

Description

[Tzuhao Kuo [:kaku]]

According to the UX spec (Bug 1305338), if a video element's video decoder is replaced by the blank video decoder (or null video decoder after Bug 1295921), we must draw black frames.

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: [WIP] Bug 1309492 part 1 - use blank decoder module again and create black frames;

Review commit: https://reviewboard.mozilla.org/r/86850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86850/

Comment 2

[Tzuhao Kuo [:kaku]]

Attachment: [WIP] Bug 1309492 part 2 - tune up the HTMLMediaElement::GetCurrentImage so that is won't return frames produced by the blank decoder module;

Review commit: https://reviewboard.mozilla.org/r/86852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86852/

Comment 3

[Tzuhao Kuo [:kaku]]

The WIP patches depend on the patches of bug 1295921.

Comment 4

[Tzuhao Kuo [:kaku]]

@Dan, 

After discussing with JW, I will try to solve this bug by reverting to the blank decoder again and make the blank decoder create black frames. (Sorry that I have to discard the null decoder module...). 

This way, the ImageContainer is filled with frames created by the blank decoder module and breaks the mechanism at bug 1295921. To fix it, I add some checks in the HTMLMediaElement::GetCurrentImage() so that the MediaDecoder::WaitOnNextRenderedVideoFrame() can only be called once and I also make the MediaDecoder::WaitOnNextRenderedVideoFrame() always wait on the monitor. If the current decoder is a normal one, the monitor is notified soon, otherwise, the monitor is notified once the seek is completed. Please refer to the 2nd WIP patch for details.

What do you think about the proposed solution? 

Actually, almost all the works could be done in the bug 1295921, however, I think you might want to land it ASAP so I suggest that we land the bug 1295921 as it is now and slightly modify the mechanism here.

Comment 5

[Tzuhao Kuo [:kaku]]

Briefly update here, the WIP patches failed at the try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=104704f5ee49

The failed test case is "test_background_video_drawimage_with_suspended_video.html" which has the following steps:
step 1: play a video and wait for the "playing" event.
step 2: upon the "playing" event, hide the video and wait for "mozbeginvideosuspend" event.
step 3: upon the "mozbeginvideosuspend" event, draw the video onto a canvas element. Since the video element's video decoder has been switched to a blank decoder, the video element has no valid frames to be drawn, so the draw operation waits for a valid frame to come.

The problem is that the blocking draw operation never finishes. 

The blocking draw operation does the following steps:
step 1: it triggers the MDSM to ask the MFR to switch the video decoder back to a normal one.
step 2: once the video decoder is switched back, it queues a SeekTask to ask the MFR to seek to the current media position, which includes (1) seek the demuxer, (2) demux the right frame and (3) decode the right frame.

With some debugging works, I found that MFR actually switches the video decoder back to a normal one successfully and it also seeks the demuxer successfully, however, it fails on the following "demux the right frame" step. In specific, the async operation here (http://searchfox.org/mozilla-central/rev/2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/dom/media/MediaFormatReader.cpp#597) never callback. It seems that the program gets into an infinite loop inside this method: http://searchfox.org/mozilla-central/rev/3e17fd777982e74daa43b1bb0c124d1cb4162370/dom/media/webm/WebMDemuxer.cpp#976 .

Will keep on figuring out...

Comment 6

[u480271]

@Kaku,

I should have replied earlier, but I'm not keen to use the blank decoder to draw black video frames. What I had in mind is that if ImageContainer doesn't contain a frame, then the compositor should draw a "transparent frame" which would allow the background colour for the website to come through. (For example, YT has a black div that sits behind the video and that what you see before the video starts playing.)

If this isn't possible, we need to flush any old frames in the image container before returning the new one. This might be possible with the refactoring in Bug 1310140 because the signalling to the main thread now happens on SeekCompleted, which calls MediaSink::Redraw (http://searchfox.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#1486)

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

We shouldn't be choosing a specific colour becuase it is guaranteed to be wrong in some places. We should either use colour from the video or the background. My view is that whatever is behind the video should be exposed. We can ask high profile sites to set the background accordingly if necessary.

Comment 8

[Tzuhao Kuo [:kaku]]

@Makr, may I have your comment on this issue? blake or transparent?

Comment 9

[Blake Wu [:bwu][:blakewu]]

Please choose me!:-)

Comment 10

[Mark Liang(:mark_liang)]

(In reply to Tzuhao Kuo [:kaku] from comment #8)
> @Mark, may I have your comment on this issue? black or transparent?

I actually prefer either black screen or using the old frames as Dan mentioned. For major video websites like Youtube or Vimeo, using black screen and transparent frame is the same since the container has black background, but when it comes to websites using video in their content, using transparent frame is going to make people feel like the video element is disappeared.

Also for websites using video as a background, using transparent frame might cause the overlaying text unreadable since we don't know for sure if the container has a background color. (White background with white text situation) The overlaying loading spinner could also have the same problem here.

Comment 11

[Tzuhao Kuo [:kaku]]

@Mark, 

Is it a good assumption that for websites using video as a background, the overlaying texts are usually in a light color?

Comment 12

[Mark Liang(:mark_liang)]

(In reply to Tzuhao Kuo [:kaku] (PTO 11/7) from comment #11)
> @Mark, 
> 
> Is it a good assumption that for websites using video as a background, the
> overlaying texts are usually in a light color?

Yes. 
By the way, comparing black screen v.s. last image frame, any big difference in performance?
Thanks.

Comment 13

[Tzuhao Kuo [:kaku]]

(In reply to Mark Liang(:mark_liang) from comment #12)
> (In reply to Tzuhao Kuo [:kaku] (PTO 11/7) from comment #11)
> > @Mark, 
> > 
> > Is it a good assumption that for websites using video as a background, the
> > overlaying texts are usually in a light color?
> 
> Yes. 
> By the way, comparing black screen v.s. last image frame, any big difference
Using the last image frame more efficient. As long as we don't update the graphics, the graphics stays at the last image frame.

Comment 14

[Tzuhao Kuo [:kaku]]

@Mark, 

Do we have an official decision to the "transition screen" proposals? The UX spec in bug 1305338 keeps in "black screen", however, I remember that you considered to switch to "last image frame"?

Comment 15

[Mark Liang(:mark_liang)]

(In reply to Tzuhao Kuo [:kaku] from comment #14)
> @Mark, 
> 
> Do we have an official decision to the "transition screen" proposals? The UX
> spec in bug 1305338 keeps in "black screen", however, I remember that you
> considered to switch to "last image frame"?

Hi,

Here's the updated spec for the transition screen:
https://mozilla.invisionapp.com/share/K48PCVSEM

We've agreed on using "last image frame".

Comment 16

[Tzuhao Kuo [:kaku]]

According to comment 15, close this bug and open bug 1346120.