Closed Bug 1346120 Opened 7 years ago Closed 7 years ago

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

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla55

Tracking Flags:

Tracking

Status

firefox55

---

fixed

People

(Reporter: kaku, Assigned: kaku)

References

(Blocks 3 open bugs)

Details

Attachments

(6 files)

Bug 1346120 part 1 - Extract BlankMediaDataDecoder so it can be shared; 7 years ago Tzuhao Kuo [:kaku] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1346120 part 2 - Implement NullDecoderModule; 7 years ago Tzuhao Kuo [:kaku] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1346120 part 3 - Use NullDecoderModule while suspending a video element's decoder; 7 years ago Tzuhao Kuo [:kaku] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1346120 part 4 - Only set ImageContainer if there are valid new images in VideoSink::RenderVideoFrames(); 7 years ago Tzuhao Kuo [:kaku] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1346120 part 5 - Revert the blank decoder to create green frames; 7 years ago Tzuhao Kuo [:kaku] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1346120 part 6 - Test drawImage gets a non-single-color image from suspended video; 7 years ago Tzuhao Kuo [:kaku] 59 bytes, text/x-review-board-request	jwwang : review+	Details

Tzuhao Kuo [:kaku]

Assignee

Description

•

7 years ago

Spawn from bug 1309492 comment 15.

Tzuhao Kuo [:kaku]

Assignee

Updated

•

7 years ago

Assignee: nobody → kaku

Blocks: 1276556, Phase_1_Shutdown_Decoder

Status: NEW → ASSIGNED

Depends on: 1295921, 1305338, 1345768

Comment 1

•

7 years ago

From bug 1305338 comment 4:
The latest UX spec: https://mozilla.invisionapp.com/share/K48PCVSEM

Tzuhao Kuo [:kaku]

Assignee

Comment 2

•

7 years ago

The upcoming patches are basically from patches-7,8,9,A of Bug 1295921 with the need of the UX spec.

Comment hidden (mozreview-request)

Change name to 'Dummy' to signify it's base for decoders that don't decode.
And we will implement a new NullDecoderModule in the next patch which will utilize the DummyMediaDataDecoder.

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

Comment hidden (mozreview-request)

The video decoder of NullDecoderModule returns a VideoData with dimension of zero size and no image data.
By this way, we reduce memory usage while a video element is suspended.

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

So that the suspended video element won't be rendered any more and keeps the last decoded frame.
This is the effect that UX specification defines.
And actually, we don't need to set ImageContainer if there are no valid new images.

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

Comment hidden (mozreview-request)

The blank decoder used to create green frames.
Bug 1274626 patch 2 modifies its behavior to return black frames.
The original implementation is better in memory usage, so we revert it.

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

Comment hidden (mozreview-request)

Once part 4 is applied, a suspend video element won't be rendered anymore, so that the video element keeps the last decoded-frame.
By this way, drawing a suspended video element to a canvas should get something that is not single-color.

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

JW Wang [:jwwang] [:jw_wang]

Comment 9

•

7 years ago

mozreview-review

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+

JW Wang [:jwwang] [:jw_wang]

Comment 10

•

7 years ago

mozreview-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+

JW Wang [:jwwang] [:jw_wang]

Comment 11

•

7 years ago

mozreview-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+

JW Wang [:jwwang] [:jw_wang]

Comment 12

•

7 years ago

mozreview-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+

JW Wang [:jwwang] [:jw_wang]

Comment 13

•

7 years ago

mozreview-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+

JW Wang [:jwwang] [:jw_wang]

Comment 14

•

7 years ago

mozreview-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+

Comment hidden (mozreview-request)

Comment on attachment 8845754 [details]
Bug 1346120 part 1 - Extract BlankMediaDataDecoder so it can be shared;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118904/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8845755 [details]
Bug 1346120 part 2 - Implement NullDecoderModule;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118906/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8845756 [details]
Bug 1346120 part 3 - Use NullDecoderModule while suspending a video element's decoder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118908/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8845757 [details]
Bug 1346120 part 4 - Only set ImageContainer if there are valid new images in VideoSink::RenderVideoFrames();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118910/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8845758 [details]
Bug 1346120 part 5 - Revert the blank decoder to create green frames;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118912/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8845759 [details]
Bug 1346120 part 6 - Test drawImage gets a non-single-color image from suspended video;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118914/diff/1-2/

Tzuhao Kuo [:kaku]

Assignee

Comment 21

•

7 years ago

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d9ed4f978b1e00d50d5dc26bd4199c215209fe3
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97c46a269379b38139e6a5bdd53443f4958ac993

Tzuhao Kuo [:kaku]

Assignee

Comment 22

•

7 years ago

Try looks pretty good, let's check in, thanks for the review!

Tzuhao Kuo [:kaku]

Assignee

Comment 23

•

7 years ago

Please help to check-in bug 1345403 comment 29 before this bug, sorry for inconvenience and thanks very much!

Keywords: checkin-needed

Pulsebot

Comment 24

•

7 years ago

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

Comment hidden (mozreview-request)

Comment on attachment 8845754 [details]
Bug 1346120 part 1 - Extract BlankMediaDataDecoder so it can be shared;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118904/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8845755 [details]
Bug 1346120 part 2 - Implement NullDecoderModule;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118906/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8845756 [details]
Bug 1346120 part 3 - Use NullDecoderModule while suspending a video element's decoder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118908/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8845757 [details]
Bug 1346120 part 4 - Only set ImageContainer if there are valid new images in VideoSink::RenderVideoFrames();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118910/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8845758 [details]
Bug 1346120 part 5 - Revert the blank decoder to create green frames;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118912/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8845759 [details]
Bug 1346120 part 6 - Test drawImage gets a non-single-color image from suspended video;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118914/diff/2-3/

Tzuhao Kuo [:kaku]

Assignee

Updated

•

7 years ago

Blocks: 1346705

Iris Hsiao [:ihsiao]

Comment 31

•

7 years ago

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)

Tzuhao Kuo [:kaku]

Assignee

Comment 32

•

7 years ago

(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)

Tzuhao Kuo [:kaku]

Assignee

Comment 33

•

7 years ago

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d58ae40ec66d357b9038019b4b0ee541f51cbc1
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ce13cf27550eabf5e753332ce7e11ac5ed082df

Tzuhao Kuo [:kaku]

Assignee

Comment 34

•

7 years ago

Try looks good, thanks for review and check in again!

Keywords: checkin-needed

Pulsebot

Comment 35

•

7 years ago

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

Carsten Book [:Tomcat]

Comment 36

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/f002383e4302
https://hg.mozilla.org/mozilla-central/rev/5e9e7264a224
https://hg.mozilla.org/mozilla-central/rev/86e79ca1a6bb
https://hg.mozilla.org/mozilla-central/rev/1f5176831c33
https://hg.mozilla.org/mozilla-central/rev/b1ce928cbeeb
https://hg.mozilla.org/mozilla-central/rev/e924204de040

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox55: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla55

Bogdan Surd, QA [:bsurd, NI]

Updated

•

7 years ago

Updated

•

7 years ago

No longer depends on: 1295921

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