Bug 149154 - CurrentTime on mediaController is set as 0 when playback is completed.
Summary: CurrentTime on mediaController is set as 0 when playback is completed.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-15 01:20 PDT by sangdeug
Modified: 2015-09-22 07:00 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.01 KB, patch)
2015-09-15 02:38 PDT, sangdeug
no flags Details | Formatted Diff | Diff
Patch (5.17 KB, patch)
2015-09-15 21:20 PDT, sangdeug
no flags Details | Formatted Diff | Diff
Patch (4.78 KB, patch)
2015-09-17 17:48 PDT, sangdeug
no flags Details | Formatted Diff | Diff
Patch (4.46 KB, patch)
2015-09-17 21:17 PDT, sangdeug
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description sangdeug 2015-09-15 01:20:37 PDT
Clear timing for current time need to be changed to next playback, not playback complete timing.

Test site :
http://www.w3.org/2010/05/video/mediaevents.html
Comment 1 sangdeug 2015-09-15 02:38:01 PDT
Created attachment 261182 [details]
Patch
Comment 2 Alexey Proskuryakov 2015-09-15 13:52:01 PDT
Can a regression test be added for this fix?
Comment 3 sangdeug 2015-09-15 21:20:43 PDT
Created attachment 261294 [details]
Patch
Comment 4 sangdeug 2015-09-15 23:17:36 PDT
Add test case for this fix. 
Check the currentTime of mediacontroller is greater than 0 on ended event.
Comment 5 Darin Adler 2015-09-16 09:45:05 PDT
Comment on attachment 261294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261294&action=review

> Source/WebCore/html/MediaController.h:157
> +    bool m_resetCurrentTimeInNextPlay;

For new code, we should initialize in the header:

    bool m_resetCurrentTimeInNextPlay { false };
Comment 6 Darin Adler 2015-09-16 09:45:54 PDT
Comment on attachment 261294 [details]
Patch

I’d like to review, but I think a media expert should review this to see if it’s done correctly.
Comment 7 Eric Carlson 2015-09-16 10:01:06 PDT
Comment on attachment 261294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261294&action=review

> Source/WebCore/html/MediaController.cpp:458
>      case ENDED:
>          eventName = eventNames().endedEvent;
> +        m_resetCurrentTimeInNextPlay = true;
>          m_clock->stop();
> -        m_clock->setCurrentTime(0);
>          m_timeupdateTimer.stop();
>          break;

Instead of adding a new instance variable to track state, can you just set current time to 0 when playback state changes from ENDED to PLAYING?

    case PLAYING:
        if (oldReadyState == ENDED)
            m_clock->setCurrentTime(0);
        eventName = eventNames().playingEvent;
        m_clock->start();
        startTimeupdateTimer();
        break;
Comment 8 sangdeug 2015-09-16 18:59:58 PDT
Comment on attachment 261294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261294&action=review

>> Source/WebCore/html/MediaController.cpp:458
>>          break;
> 
> Instead of adding a new instance variable to track state, can you just set current time to 0 when playback state changes from ENDED to PLAYING?
> 
>     case PLAYING:
>         if (oldReadyState == ENDED)
>             m_clock->setCurrentTime(0);
>         eventName = eventNames().playingEvent;
>         m_clock->start();
>         startTimeupdateTimer();
>         break;

I tried to using oldPlaybackState but during my test, sometimes I observed that the WAITING event is comming between ENDED and PLAYING. 

e.g) 
ENDED -> PLAYING
ENDED -> WAITING -> PLAYING

To cover both case, I added a new variable to track state.

>> Source/WebCore/html/MediaController.h:157
>> +    bool m_resetCurrentTimeInNextPlay;
> 
> For new code, we should initialize in the header:
> 
>     bool m_resetCurrentTimeInNextPlay { false };

It's initialized in constructor, do I need initialize in head also?
Comment 9 Eric Carlson 2015-09-17 07:33:49 PDT
Comment on attachment 261294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261294&action=review

>>> Source/WebCore/html/MediaController.cpp:458
>>>          break;
>> 
>> Instead of adding a new instance variable to track state, can you just set current time to 0 when playback state changes from ENDED to PLAYING?
>> 
>>     case PLAYING:
>>         if (oldReadyState == ENDED)
>>             m_clock->setCurrentTime(0);
>>         eventName = eventNames().playingEvent;
>>         m_clock->start();
>>         startTimeupdateTimer();
>>         break;
> 
> I tried to using oldPlaybackState but during my test, sometimes I observed that the WAITING event is comming between ENDED and PLAYING. 
> 
> e.g) 
> ENDED -> PLAYING
> ENDED -> WAITING -> PLAYING
> 
> To cover both case, I added a new variable to track state.

OK. In any case, you should handle this in the switch statement:

    case PLAYING:
        if (m_resetCurrentTimeInNextPlay)
            m_clock->setCurrentTime(0);
        m_resetCurrentTimeInNextPlay = false;
        eventName = eventNames().playingEvent;
        m_clock->start();
        startTimeupdateTimer();
        break;

>>> Source/WebCore/html/MediaController.h:157
>>> +    bool m_resetCurrentTimeInNextPlay;
>> 
>> For new code, we should initialize in the header:
>> 
>>     bool m_resetCurrentTimeInNextPlay { false };
> 
> It's initialized in constructor, do I need initialize in head also?

For new code, the WebKit style is initialize in the header instead of in the constructor.
Comment 10 sangdeug 2015-09-17 17:48:19 PDT
Created attachment 261465 [details]
Patch
Comment 11 sangdeug 2015-09-17 17:49:15 PDT
Comment on attachment 261294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261294&action=review

>>>> Source/WebCore/html/MediaController.cpp:458
>>>>          break;
>>> 
>>> Instead of adding a new instance variable to track state, can you just set current time to 0 when playback state changes from ENDED to PLAYING?
>>> 
>>>     case PLAYING:
>>>         if (oldReadyState == ENDED)
>>>             m_clock->setCurrentTime(0);
>>>         eventName = eventNames().playingEvent;
>>>         m_clock->start();
>>>         startTimeupdateTimer();
>>>         break;
>> 
>> I tried to using oldPlaybackState but during my test, sometimes I observed that the WAITING event is comming between ENDED and PLAYING. 
>> 
>> e.g) 
>> ENDED -> PLAYING
>> ENDED -> WAITING -> PLAYING
>> 
>> To cover both case, I added a new variable to track state.
> 
> OK. In any case, you should handle this in the switch statement:
> 
>     case PLAYING:
>         if (m_resetCurrentTimeInNextPlay)
>             m_clock->setCurrentTime(0);
>         m_resetCurrentTimeInNextPlay = false;
>         eventName = eventNames().playingEvent;
>         m_clock->start();
>         startTimeupdateTimer();
>         break;

Done.

>>>> Source/WebCore/html/MediaController.h:157
>>>> +    bool m_resetCurrentTimeInNextPlay;
>>> 
>>> For new code, we should initialize in the header:
>>> 
>>>     bool m_resetCurrentTimeInNextPlay { false };
>> 
>> It's initialized in constructor, do I need initialize in head also?
> 
> For new code, the WebKit style is initialize in the header instead of in the constructor.

Done.
Comment 12 Eric Carlson 2015-09-17 18:49:46 PDT
Comment on attachment 261465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261465&action=review

r=me with one minor nit.

Thanks!

> Source/WebCore/html/MediaController.cpp:60
> +    , m_resetCurrentTimeInNextPlay(false)

Nit: this is unnecessary because you have already initialized it in the header.
Comment 13 sangdeug 2015-09-17 21:17:06 PDT
Created attachment 261479 [details]
Patch
Comment 14 Eric Carlson 2015-09-18 10:37:23 PDT
Comment on attachment 261479 [details]
Patch

Thank you!
Comment 15 Eric Carlson 2015-09-21 10:59:12 PDT
sangdeug - if you want someone to land your patch, mark it "cq?" and any committer can land it manually or mark it "cq+" and the commit bot will land it automatically.
Comment 16 WebKit Commit Bot 2015-09-21 16:29:33 PDT
Comment on attachment 261479 [details]
Patch

Rejecting attachment 261479 [details] from commit-queue.

sangdeug.kim@samsung.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 17 WebKit Commit Bot 2015-09-21 16:30:45 PDT
Comment on attachment 261479 [details]
Patch

Rejecting attachment 261479 [details] from commit-queue.

sangdeug.kim@samsung.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 18 WebKit Commit Bot 2015-09-22 06:59:57 PDT
Comment on attachment 261479 [details]
Patch

Clearing flags on attachment: 261479

Committed r190114: <http://trac.webkit.org/changeset/190114>
Comment 19 WebKit Commit Bot 2015-09-22 07:00:02 PDT
All reviewed patches have been landed.  Closing bug.