Closed Bug 1346120 Opened 3 years ago Closed 3 years ago

If a video element's video is suspended, keep the last decoded video frame on the screen

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

(Blocks 3 open bugs)

Details

Attachments

(6 files)

Spawn from bug 1309492 comment 15.
Assignee: nobody → kaku
Status: NEW → ASSIGNED
Depends on: 1295921, 1305338, 1345768
See Also: → 1309492
The upcoming patches are basically from patches-7,8,9,A of Bug 1295921 with the need of the UX spec.
Comment on attachment 8845754 [details]
Bug 1346120 part 1 - Extract BlankMediaDataDecoder so it can be shared;

https://reviewboard.mozilla.org/r/118904/#review120868

::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:7
(Diff revision 1)
>  /* vim:set ts=2 sw=2 sts=2 et cindent: */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
>  
> +#include "DummyMediaDataDecoder.h"

The sorting should be case sensitive. Please put paths starting with upper-case characters together.
Attachment #8845754 - Flags: review?(jwwang) → review+
Comment on attachment 8845755 [details]
Bug 1346120 part 2 - Implement NullDecoderModule;

https://reviewboard.mozilla.org/r/118906/#review120870
Attachment #8845755 - Flags: review?(jwwang) → review+
Comment on attachment 8845756 [details]
Bug 1346120 part 3 - Use NullDecoderModule while suspending a video element's decoder;

https://reviewboard.mozilla.org/r/118908/#review120872
Attachment #8845756 - Flags: review?(jwwang) → review+
Comment on attachment 8845757 [details]
Bug 1346120 part 4 - Only set ImageContainer if there are valid new images in VideoSink::RenderVideoFrames();

https://reviewboard.mozilla.org/r/118910/#review120874

::: dom/media/mediasink/VideoSink.cpp:404
(Diff revision 1)
>  
>      VSINK_LOG_V("playing video frame %" PRId64 " (id=%x) (vq-queued=%" PRIuSIZE ")",
>                  frame->mTime, frame->mFrameID, VideoQueue().GetSize());
>    }
> +
> +  if (images.Length()) {

Prefer |foo > 0| over |foo| when it comes to integer comparison.
Attachment #8845757 - Flags: review?(jwwang) → review+
Comment on attachment 8845758 [details]
Bug 1346120 part 5 - Revert the blank decoder to create green frames;

https://reviewboard.mozilla.org/r/118912/#review120876
Attachment #8845758 - Flags: review?(jwwang) → review+
Comment on attachment 8845759 [details]
Bug 1346120 part 6 - Test drawImage gets a non-single-color image from suspended video;

https://reviewboard.mozilla.org/r/118914/#review120878

::: dom/media/test/test_background_video_drawimage_with_suspended_video.html:50
(Diff revision 1)
> +  for (let i = 0; i < 4*4; i++) {
> +    let r = pixels[4*i+0];
> +    let g = pixels[4*i+1];
> +    let b = pixels[4*i+2];
> +    if (r != rr || g != gg || b != bb) {
> +      allSame = false;

Early break when 2 pixels are not the same color.
Attachment #8845759 - Flags: review?(jwwang) → review+
Try looks pretty good, let's check in, thanks for the review!
Please help to check-in bug 1345403 comment 29 before this bug, sorry for inconvenience and thanks very much!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/94a483ef784e
part 1 - Extract BlankMediaDataDecoder so it can be shared; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/c0758b9bf7b5
part 2 - Implement NullDecoderModule; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/ba579adbed21
part 3 - Use NullDecoderModule while suspending a video element's decoder; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/98d212462786
part 4 - Only set ImageContainer if there are valid new images in VideoSink::RenderVideoFrames(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f16556658fd9
part 5 - Revert the blank decoder to create green frames; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/a30c73fc8d69
part 6 - Test drawImage gets a non-single-color image from suspended video; r=jwwang
Keywords: checkin-needed
Blocks: 1346705
sorry had to back this out for autophone Mdm tests failure in test_background_video_drawimage_with_suspended_video.html like https://treeherder.mozilla.org/logviewer.html#?job_id=83329627&repo=autoland&lineNumber=806

https://hg.mozilla.org/integration/autoland/rev/a7f05a91241e
Flags: needinfo?(kaku)
(In reply to Iris Hsiao [:ihsiao] from comment #31)
> sorry had to back this out for autophone Mdm tests failure in
> test_background_video_drawimage_with_suspended_video.html like
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=83329627&repo=autoland&lineNumber=806
> 
> https://hg.mozilla.org/integration/autoland/rev/a7f05a91241e
comment 30 solves this issue.
Flags: needinfo?(kaku)
Try looks good, thanks for review and check in again!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f002383e4302
part 1 - Extract BlankMediaDataDecoder so it can be shared; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5e9e7264a224
part 2 - Implement NullDecoderModule; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/86e79ca1a6bb
part 3 - Use NullDecoderModule while suspending a video element's decoder; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/1f5176831c33
part 4 - Only set ImageContainer if there are valid new images in VideoSink::RenderVideoFrames(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/b1ce928cbeeb
part 5 - Revert the blank decoder to create green frames; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/e924204de040
part 6 - Test drawImage gets a non-single-color image from suspended video; r=jwwang
Keywords: checkin-needed
See Also: → 1348864
No longer depends on: 1295921
You need to log in before you can comment on or make changes to this bug.