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

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Updated

2 years ago
Assignee: nobody → kaku
Blocks: 1276556, 1293963
Depends on: 1305338
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.
Flags: needinfo?(kaku)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
(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!
Flags: needinfo?(kaku)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Depends on: 1295921
(Assignee)

Comment 8

2 years ago
The patches depend on the patches of bug 1295921.

Comment 9

2 years ago
mozreview-review
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.
Attachment #8802084 - Flags: review?(jwwang) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8802084 - Attachment is obsolete: true

Comment 12

2 years ago
mozreview-review
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.
(Assignee)

Comment 13

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
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.
Attachment #8802086 - Flags: review?(jaws) → review+
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.
Attachment #8802085 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

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

https://reviewboard.mozilla.org/r/119628/#review121798
Attachment #8846913 - Flags: review?(jwwang) → review+
(Assignee)

Comment 24

2 years ago
Thanks for the review!
Keywords: checkin-needed

Comment 25

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c02436238eee
https://hg.mozilla.org/mozilla-central/rev/32f6036fe3fd
https://hg.mozilla.org/mozilla-central/rev/eb1c6835ffe8
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

2 years ago
Blocks: 1350852
(Assignee)

Updated

2 years ago
No longer depends on: 1295921
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(kaku)
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Flags: needinfo?(adrian.florinescu)
Resolution: --- → FIXED
Depends on: 1359815
Opened the dependence bug 1359815 as per :jaws advice from comment 28. 
:kaku, could you please take a look at that issue?
Flags: needinfo?(adrian.florinescu)
(Assignee)

Comment 30

2 years ago
:AdrianSV, 
bug 1359815 has landed, could you help to verify?
Flags: needinfo?(kaku) → needinfo?(adrian.florinescu)
(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.
Flags: needinfo?(adrian.florinescu)
You need to log in before you can comment on or make changes to this bug.