New issue
Advanced search Search tips

Issue 675556 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Video stops updating currentTime after setting currentTime backward, executing video.pause(), and then executing video.play().

Reported by kenji.im...@gmail.com, Dec 19 2016

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36

Steps to reproduce the problem:
1. Visit http://www.w3.org/2010/05/video/mediaevents.html
2. Open developer tools and paste the below snippet in the console

```
var video = document.querySelector('video');

// play video for 5 seconds
video.play();
setTimeout(function() {
  // then set currentTime to 0, pause video, and play again
  video.currentTime = 0;
  video.pause();
  video.play();
}, 5000);
```

3. Watch 'timeupdate' and 'currentTime' fields.

What is the expected behavior?

What went wrong?
Video stops updating currentTime and firing timeupdate event after the function passed to setTimeout in the above snippet executed.
It comes back to normal when it reaches to the time when its currentTime has been set (~5 sec).

Did this work before? Yes 54

Does this work in other browsers? Yes

Chrome version: 55.0.2883.95  Channel: stable
OS Version: OS X 10.11.3
Flash Version: Shockwave Flash 24.0 r0
 
Labels: M-55
Components: Blink>Media>Video
Labels: OS-Chrome OS-Linux OS-Windows
Reproduced in 55.0.2883.95, as well as Canary 57.0.2956.0. Interestingly, the minute I add a console.log() command in between video.currentTime = 0; and pausing the video, it works again. I think this might be a timing issue, perhaps trying to pause too soon after the video was scrubbed programmatically doesn't work? Pausing before the video is scrubbed also works. So you can use this as a workaround for now:

```
var video = document.querySelector('video');

// play video for 5 seconds
video.play();
setTimeout(function() {
  // pause video, then set currentTime to 0 and play again
  video.pause();
  video.currentTime = 0;
  video.play();
}, 5000);
```

Sending to the blink triage queue.
Thanks for the workaround.
It works for me.
Cc: kkaluri@chromium.org
Labels: -Pri-2 -M-55 hasbisect-per-revision ReleaseBlock-Stable M-57 Pri-1
Owner: alokp@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce this issue on windows 10, Ubuntu 14.04 and Mac 10.12.2 on latest chrome stable #55.0.2883.87 and Canary #57.0.2955.0	
Issue is broken in M55. 


Bisect Info:
===========
Good build : 55.0.2855.0,  Revision Range- 417475
Bad build  : 55.0.2857.0,  Revision Range- 417845

After executing the per-revision-bisect script, i got the following CL's between good and bad build versions
============================================
https://chromium.googlesource.com/chromium/src/+log/db476ca7de669dd97607ceeabf6f97fbc3201f1e..a452e2357b4cbd498bb728fb7b5f4006123d111d

The suspecting Change Log is :
-----------
https://chromium.googlesource.com/chromium/src/+/a452e2357b4cbd498bb728fb7b5f4006123d111d

From the above CL suspecting the below change
--------------------------------------
Review-Url: https://codereview.chromium.org/2239243002

alokp@- Could you please look into this issue, if it's related to your change?  if not could you please help us to reassign this issue to the right owner.

Thank You...
Cc: chcunningham@chromium.org
Chris, could it be related to the currentTime change you landed recently?

Comment 6 by alokp@chromium.org, Dec 20 2016

Cc: -chcunningham@chromium.org
Owner: chcunningham@chromium.org
The patch referenced in comment 4 only affects mojo media pipeline, which is only used on chromecast. Bouncing to chcunningham@ to take a look.
Status: Started (was: Assigned)
Taking a look.
The bisect had it right. The changes to this file are common to mojo and non-mojo. https://codereview.chromium.org/2239243002/diff/340001/media/base/pipeline_impl.cc

Fix out for review
https://codereview.chromium.org/2616703002/
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/454da202ef172f3ecbfb48c154ff97c8c10011b0

commit 454da202ef172f3ecbfb48c154ff97c8c10011b0
Author: chcunningham <chcunningham@chromium.org>
Date: Thu Jan 05 00:28:25 2017

Fix race condition in media Pipeline::GetMediaTime() after seeking.

Seeking posts a task from main thread to media thread to seek the
renderer. Calling GetMediaTime() should be safe even when the
renderer has not yet received the message to performed the seek.

This change gaurds against using the renderer's media time while
a seek is pending. This prevents polluting |last_media_time_|
with bad values from the renderer that doesn't yet know about
the seek.

BUG= 675556 
TEST=New unit test && manually verfied bug fix.

Review-Url: https://codereview.chromium.org/2616703002
Cr-Commit-Position: refs/heads/master@{#441526}

[modify] https://crrev.com/454da202ef172f3ecbfb48c154ff97c8c10011b0/media/base/pipeline_impl.cc
[modify] https://crrev.com/454da202ef172f3ecbfb48c154ff97c8c10011b0/media/base/pipeline_impl.h
[modify] https://crrev.com/454da202ef172f3ecbfb48c154ff97c8c10011b0/media/base/pipeline_impl_unittest.cc

I'll let this bake a few days in Canary before requesting a merge back.
Labels: TE-Verified-M57 TE-Verified-57.0.2976.5
Verified this issue on Windows-10, Mac OS 10.12.2 and Ubuntu 14.04 using chrome latest M57 #57.0.2976.5 by following steps mentioned in the comment #0.

Observed that 'timeupdate' and 'currentTime' fields are updating according to script provided in comment #0
Please find the attached screen-cast for the same.

Adding TE-Verified labels.
Issue 675556.mp4
5.8 MB View Download
Labels: Merge-Request-56
Project Member

Comment 13 by sheriffbot@chromium.org, Jan 10 2017

Labels: -Merge-Request-56 Hotlist-Merge-Approved Merge-Approved-56
Your change meets the bar and is auto-approved for M56. Please go ahead and merge the CL manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 14 by bugdroid1@chromium.org, Jan 10 2017

Labels: -merge-approved-56 merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f683e0741087dbe0ddf4ed5d878bc72b61246ff8

commit f683e0741087dbe0ddf4ed5d878bc72b61246ff8
Author: Chris Cunningham <chcunningham@chromium.org>
Date: Tue Jan 10 22:03:48 2017

[TO 56] Fix race in media Pipeline::GetMediaTime() after seeking.

Seeking posts a task from main thread to media thread to seek the
renderer. Calling GetMediaTime() should be safe even when the
renderer has not yet received the message to performed the seek.

This change gaurds against using the renderer's media time while
a seek is pending. This prevents polluting |last_media_time_|
with bad values from the renderer that doesn't yet know about
the seek.

BUG= 675556 
TEST=New unit test && manually verfied bug fix.
TBR=sandersd@chromium.org

Review-Url: https://codereview.chromium.org/2616703002
Cr-Commit-Position: refs/heads/master@{#441526}
(cherry picked from commit 454da202ef172f3ecbfb48c154ff97c8c10011b0)

Review-Url: https://codereview.chromium.org/2621183002 .
Cr-Commit-Position: refs/branch-heads/2924@{#722}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/f683e0741087dbe0ddf4ed5d878bc72b61246ff8/media/base/pipeline_impl.cc
[modify] https://crrev.com/f683e0741087dbe0ddf4ed5d878bc72b61246ff8/media/base/pipeline_impl.h
[modify] https://crrev.com/f683e0741087dbe0ddf4ed5d878bc72b61246ff8/media/base/pipeline_impl_unittest.cc

Status: Fixed (was: Started)
Cc: rbasuvula@chromium.org
Labels: TE-Verified-M56 TE-Verified-56.0.2924.59
Tested the issue on Ubuntu 14.04,Win 7 and Mac OS 10.12.2 using chrome latest Beta M56-56.0.2924.59 by following steps mentioned in the original comment. Observed that 'timeupdate' and 'currentTime' fields are updating according to script provided in comment #0. Hence adding TE-Verified label.

Please find the screen shot for reference.

Thank you!
675556.ogv
4.6 MB View Download
Cc: mlamouri@chromium.org chcunningham@chromium.org dalecur...@chromium.org
 Issue 680256  has been merged into this issue.
Status: Verified (was: Fixed)

Sign in to add a comment