Bug 162575 - Seeking video doesn't update seek position
Summary: Seeking video doesn't update seek position
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-09-26 13:16 PDT by Wenson Hsieh
Modified: 2016-09-26 15:44 PDT (History)
5 users (show)

See Also:


Attachments
Patch (27.55 KB, patch)
2016-09-26 13:30 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix unused function on non-media-remote builds. (27.52 KB, patch)
2016-09-26 13:45 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.53 MB, application/zip)
2016-09-26 14:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.54 MB, application/zip)
2016-09-26 14:42 PDT, Build Bot
no flags Details
Further simplified the patch. (12.42 KB, patch)
2016-09-26 14:57 PDT, Wenson Hsieh
jer.noble: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2016-09-26 13:16:47 PDT
Seeking video doesn't update seek position
Comment 1 Wenson Hsieh 2016-09-26 13:17:18 PDT
<rdar://problem/28457219>
Comment 2 Wenson Hsieh 2016-09-26 13:30:50 PDT
Created attachment 289858 [details]
Patch
Comment 3 Wenson Hsieh 2016-09-26 13:45:11 PDT
Created attachment 289865 [details]
Fix unused function on non-media-remote builds.
Comment 4 Build Bot 2016-09-26 14:38:47 PDT
Comment on attachment 289865 [details]
Fix unused function on non-media-remote builds.

Attachment 289865 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2150151

New failing tests:
media/remote-control-command-is-user-gesture.html
media/remote-control-command-seek.html
Comment 5 Build Bot 2016-09-26 14:38:50 PDT
Created attachment 289872 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 6 Build Bot 2016-09-26 14:42:48 PDT
Comment on attachment 289865 [details]
Fix unused function on non-media-remote builds.

Attachment 289865 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2150159

New failing tests:
media/remote-control-command-is-user-gesture.html
media/remote-control-command-seek.html
Comment 7 Build Bot 2016-09-26 14:42:51 PDT
Created attachment 289873 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 Wenson Hsieh 2016-09-26 14:57:26 PDT
Created attachment 289878 [details]
Further simplified the patch.
Comment 9 Jer Noble 2016-09-26 15:12:25 PDT
Comment on attachment 289878 [details]
Further simplified the patch.

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

r=me, with nits

> Source/WebCore/ChangeLog:20
> +        Also, when responding to a remote seek, perform the seek with no tolerance. This prevents the playhead from
> +        stuttering at the end of a seek from the final requested destination of the seek to the last actually seeked
> +        time in the video.

We should add a FIXME about this, because it doesn't match the behavior of the inline controls.  For the inline controls, we do a fastSeek() for 'mousedown' and 'mousemove' and a zero-tolerance seek for 'mouseup'.  We'd need an explicit "end seeking" command to implement this though.

> Source/WebCore/platform/audio/PlatformMediaSessionManager.h:113
> +    void reloadSupportedCommands() const;

This is probably from a different patch.

> Source/WebCore/platform/audio/mac/MediaSessionManagerMac.h:-57
> -    bool m_isInBackground { false };

Is this necessary?
Comment 10 Wenson Hsieh 2016-09-26 15:41:06 PDT
Comment on attachment 289878 [details]
Further simplified the patch.

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

>> Source/WebCore/ChangeLog:20
>> +        time in the video.
> 
> We should add a FIXME about this, because it doesn't match the behavior of the inline controls.  For the inline controls, we do a fastSeek() for 'mousedown' and 'mousemove' and a zero-tolerance seek for 'mouseup'.  We'd need an explicit "end seeking" command to implement this though.

FIXME added. Thanks!

>> Source/WebCore/platform/audio/PlatformMediaSessionManager.h:113
>> +    void reloadSupportedCommands() const;
> 
> This is probably from a different patch.

Oh, I meant to remove this! Done.

>> Source/WebCore/platform/audio/mac/MediaSessionManagerMac.h:-57
>> -    bool m_isInBackground { false };
> 
> Is this necessary?

True -- we don't need to remove this. Added back in.
Comment 11 Wenson Hsieh 2016-09-26 15:44:42 PDT
Committed r206399: <http://trac.webkit.org/changeset/206399>