Closed Bug 1309494 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

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: nobody → kaku
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)
(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)
Depends on: 1295921
The patches depend on the patches of bug 1295921.
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-
Attachment #8802084 - Attachment is obsolete: true
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 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 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 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+
Thanks for the review!
Keywords: checkin-needed
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
Blocks: 1350852
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
Closed: 5 years ago5 years ago
Flags: needinfo?(adrian.florinescu)
Resolution: --- → FIXED
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)
: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.