1309494 - Use spinning loading UI if resuming video element's video decoder is longer than 250ms.

Status: RESOLVED FIXED

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

Description

[Tzuhao Kuo [:kaku]]

According to the UX spec (Bug 1305338), we must use spinning loading UI if resuming video element's video decoder is longer than 250ms.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Please talk to Ray (CC'd) if you need help understanding the front-end video control code. Keep in mind that we are updating it currently in bug 1271765; please above future rebase when possible.

Comment 2

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1309494 part 1 - add mozvideodecoderresumed event;

mozvideodecoderresumed is used to indicate UI that video decoder has been resumed.

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

Comment 3

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1309494 part 1 - let video controler handle the "mozexitvideosuspend" event;

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

Comment 4

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1309494 part 2 - implement the show throbber mechanism;

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

Comment 5

[Tzuhao Kuo [:kaku]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #1)
> Please talk to Ray (CC'd) if you need help understanding the front-end video
> control code. Keep in mind that we are updating it currently in bug 1271765;
> please above future rebase when possible.
Thanks for reminding, have told to Ray, he gave me great hints!

Comment 6

[Tzuhao Kuo [:kaku]]

Comment on attachment 8802085 [details]
Bug 1309494 part 1 - let video controler handle the "mozexitvideosuspend" event;

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

Comment 7

[Tzuhao Kuo [:kaku]]

Comment on attachment 8802086 [details]
Bug 1309494 part 2 - implement the show throbber mechanism;

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

Comment 8

[Tzuhao Kuo [:kaku]]

The patches depend on the patches of bug 1295921.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8802084 [details]
Bug 1309494 part 1 - add mozvideodecoderresumed event;

https://reviewboard.mozilla.org/r/86626/#review85806

::: dom/media/MediaDecoderStateMachine.h:127
(Diff revision 1)
>    Invalidate,
>    EnterVideoSuspend,
>    ExitVideoSuspend,
>    BeginVideoSuspend,
> -  CancelVideoSuspend
> +  CancelVideoSuspend,
> +  VideoDecoderResumed

I don't think you need to add a new event.

1. 'mozexitvideosuspend' is fired when seek begins.
2. 'canplay' is fire when seek is done.

These are the 2 events you need.

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8802085 [details]
Bug 1309494 part 1 - let video controler handle the "mozexitvideosuspend" event;

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

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8802086 [details]
Bug 1309494 part 2 - implement the show throbber mechanism;

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

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8802086 [details]
Bug 1309494 part 2 - implement the show throbber mechanism;

https://reviewboard.mozilla.org/r/86630/#review85814

::: toolkit/content/widgets/videocontrols.xml:639
(Diff revision 3)
>                              break;
>                          case "seeked":
>                          case "playing":
>                          case "canplay":
>                          case "canplaythrough":
> +                            if (!!this._showThrobberTimer ||

Please explain why listening to these events to start/stop throbber.

Comment 13

[Tzuhao Kuo [:kaku]]

Comment on attachment 8802086 [details]
Bug 1309494 part 2 - implement the show throbber mechanism;

https://reviewboard.mozilla.org/r/86630/#review85814

> Please explain why listening to these events to start/stop throbber.

Roger that.

Comment 14

[Tzuhao Kuo [:kaku]]

Comment on attachment 8802086 [details]
Bug 1309494 part 2 - implement the show throbber mechanism;

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

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8802086 [details]
Bug 1309494 part 2 - implement the show throbber mechanism;

https://reviewboard.mozilla.org/r/86630/#review86614

::: toolkit/content/widgets/videocontrols.xml:490
(Diff revision 4)
> +                 * meanwhile, we also file a "mozexitvideosuspend" event which
> +                 * we used to start the timer here.
> +                 *
> +                 * Once the queued seek operation is done, we file a "canplay"

Please use the word "dispatch" instead of "file".

::: toolkit/content/widgets/videocontrols.xml:494
(Diff revision 4)
> +                 * seek the resumed video decoder to the current position;
> +                 * meanwhile, we also file a "mozexitvideosuspend" event which
> +                 * we used to start the timer here.
> +                 *
> +                 * Once the queued seek operation is done, we file a "canplay"
> +                 * event which indicating that the resuming operation is

"indicates" instead of "indicating"

::: toolkit/content/widgets/videocontrols.xml:499
(Diff revision 4)
> +                 * event which indicating that the resuming operation is
> +                 * completed.
> +                 */
> +                SHOW_THROBBER_TIMEOUT_MS: 250,
> +                _showThrobberTimer: null,
> +                _showThrobberWhileResumingVideoDecoder: false,

We can get rid of the _showThrobberWhileResumingVideoDecoder variable. Instead of checking this variable we can just check if _showThrobberTimer is not null.

::: toolkit/content/widgets/videocontrols.xml:508
(Diff revision 4)
> +                        this.statusIcon.setAttribute("type", "throbber");
> +                        this.setupStatusFader();
> +                    }, this.SHOW_THROBBER_TIMEOUT_MS);
> +                },
> +                _cancelShowThrobberWhileResumingVideoDecoder: function() {
> +                    if (!!this._showThrobberTimer) {

Please remove `!!` here because null or false will both evaluate to false.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8802085 [details]
Bug 1309494 part 1 - let video controler handle the "mozexitvideosuspend" event;

https://reviewboard.mozilla.org/r/86628/#review86616

::: toolkit/content/widgets/videocontrols.xml:291
(Diff revision 3)
>                  videoEvents : ["play", "pause", "ended", "volumechange", "loadeddata",
>                                 "loadstart", "timeupdate", "progress",
>                                 "playing", "waiting", "canplay", "canplaythrough",
>                                 "seeking", "seeked", "emptied", "loadedmetadata",
>                                 "error", "suspend", "stalled",
> -                               "mozinterruptbegin", "mozinterruptend" ],
> +                               "mozinterruptbegin", "mozinterruptend",

Please remove the mozinterruptbegin and mozinterruptEnd since there is nothing to do with them.

::: toolkit/content/widgets/videocontrols.xml:635
(Diff revision 3)
>                          case "mozinterruptbegin":
>                          case "mozinterruptend":
>                              // Nothing to do...
>                              break;

Same here, please remove this code.

Comment 17

[Tzuhao Kuo [:kaku]]

Comment on attachment 8802085 [details]
Bug 1309494 part 1 - let video controler handle the "mozexitvideosuspend" event;

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

Comment 18

[Tzuhao Kuo [:kaku]]

Comment on attachment 8802086 [details]
Bug 1309494 part 2 - implement the show throbber mechanism;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86630/diff/4-5/

Comment 19

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1309494 part 0 - make the seek operation at StateObject::HandleResumeVideoDecoding() observable;

So that the we can listen to the 'canplay' event in the videocontrols.xml.

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

Comment 20

[Tzuhao Kuo [:kaku]]

Comment on attachment 8802085 [details]
Bug 1309494 part 1 - let video controler handle the "mozexitvideosuspend" event;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86628/diff/4-5/

Comment 21

[Tzuhao Kuo [:kaku]]

Comment on attachment 8802086 [details]
Bug 1309494 part 2 - implement the show throbber mechanism;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86630/diff/5-6/

Comment 22

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8846913 [details]
Bug 1309494 part 0 - make the seek operation at StateObject::HandleResumeVideoDecoding() observable;

https://reviewboard.mozilla.org/r/119628/#review121798

Comment 23

[Tzuhao Kuo [:kaku]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c794b6cbf4d26edf7d70e13e64e867b24ba04983

Comment 24

[Tzuhao Kuo [:kaku]]

Thanks for the review!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c02436238eee
part 0 - make the seek operation at StateObject::HandleResumeVideoDecoding() observable; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/32f6036fe3fd
part 1 - let video controler handle the "mozexitvideosuspend" event; r=jaws
https://hg.mozilla.org/integration/autoland/rev/eb1c6835ffe8
part 2 - implement the show throbber mechanism; r=jaws

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c02436238eee
https://hg.mozilla.org/mozilla-central/rev/32f6036fe3fd
https://hg.mozilla.org/mozilla-central/rev/eb1c6835ffe8

Comment 27

[Adrian Florinescu [:aflorinescu]]

version: 55.0a1 20170423100233
OS: Win, Mac10.12, Win10

Reopening this issue, since I do not see any spinning loading UI, even when apparently the video resume takes longer. 
I'm not sure if indeed the video decoder resume is longer that 250ms (clues how to check this?), but I'm running a testcase in which I have 6-7 full HD videos.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Since the patches for this bug landed a month ago, please do not re-open the bug. In this case, it would be better to file a new bug and mark that new bug as "blocking" this bug. We generally only re-open a bug if the patches are backed out.

Comment 29

[Adrian Florinescu [:aflorinescu]]

Opened the dependence bug 1359815 as per :jaws advice from comment 28. 
:kaku, could you please take a look at that issue?

Comment 30

[Tzuhao Kuo [:kaku]]

:AdrianSV, 
bug 1359815 has landed, could you help to verify?

Comment 31

[Adrian Florinescu [:aflorinescu]]

(In reply to Tzuhao Kuo [:kaku] from comment #30)
> :AdrianSV, 
> bug 1359815 has landed, could you help to verify?

Yes, already all the information in on the bug 1359815.