Bug 71341 - Add Clock class and platform-specific implementations.
Summary: Add Clock class and platform-specific implementations.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks: 71408
  Show dependency treegraph
 
Reported: 2011-11-01 17:19 PDT by Jer Noble
Modified: 2011-11-03 11:05 PDT (History)
5 users (show)

See Also:


Attachments
Add Clock, PlatformClockCA, and PlatformClockPOSIX (24.93 KB, patch)
2011-11-02 11:20 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
MediaController and HTMLMediaElement changes. (71.56 KB, patch)
2011-11-02 13:31 PDT, Jer Noble
abarth: review-
Details | Formatted Diff | Diff
Source/JavaScriptCore: Implement MediaController. (24.91 KB, patch)
2011-11-02 16:34 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Source/WebCore: Implement MediaController. (70.14 KB, patch)
2011-11-02 16:34 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (24.93 KB, patch)
2011-11-02 16:38 PDT, Jer Noble
sam: review+
Details | Formatted Diff | Diff
Patch (70.11 KB, patch)
2011-11-02 16:40 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-11-01 17:19:37 PDT
Implement MediaController.
Comment 1 Jer Noble 2011-11-02 11:20:28 PDT
Created attachment 113339 [details]
Add Clock, PlatformClockCA, and PlatformClockPOSIX
Comment 2 Jer Noble 2011-11-02 13:31:17 PDT
Created attachment 113364 [details]
MediaController and HTMLMediaElement changes.
Comment 3 Adam Barth 2011-11-02 14:49:50 PDT
Comment on attachment 113339 [details]
Add Clock, PlatformClockCA, and PlatformClockPOSIX

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

We should be able to do this without virtual functions.

> Source/WebCore/platform/Clock.cpp:44
> +PassRefPtr<Clock> Clock::create()
> +{
> +#if USE(COREAUDIO)
> +    return adoptRef(new PlatformClockCA());
> +#else
> +    return adoptRef(new PlatformClockPOSIX());
> +#endif
> +}

Would it make sense to move Clock::create to PlatformClockCA.cpp and PlatformClockPOSIX.cpp, respectively?

> Source/WebCore/platform/Clock.h:47
> +    virtual void setCurrentTime(float) = 0;
> +    virtual float currentTime() const = 0;
> +
> +    virtual void setPlayRate(float) = 0;
> +    virtual float playRate() const = 0;
> +
> +    virtual void start() = 0;
> +    virtual void stop() = 0;
> +    virtual bool isRunning() const = 0;

Why virtual functions?  If only one implementation will be linked in at a time, there's no need for virtual functions.
Comment 4 Adam Barth 2011-11-02 14:51:22 PDT
Comment on attachment 113364 [details]
MediaController and HTMLMediaElement changes.

This patch is based on an old version of WebKit.  Can you merge this to top-of-tree so we can see what the patch will look like when landed?
Comment 5 Adam Barth 2011-11-02 14:51:54 PDT
Also, we prefer to have one patch per bug.  If you'd like to land these separately (which I think makes sense) please consider using multiple bugs.
Comment 6 Jer Noble 2011-11-02 15:01:28 PDT
(In reply to comment #3)
> (From update of attachment 113339 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113339&action=review
> 
> We should be able to do this without virtual functions.

> Would it make sense to move Clock::create to PlatformClockCA.cpp and PlatformClockPOSIX.cpp, respectively?

See below. :)

> Why virtual functions?  If only one implementation will be linked in at a time, there's no need for virtual functions.

There may be more than one implementation at a time.  We are working on a clock subclass which can synchronize AVFoundation-based media elements, but would still need a CA or POSIX clock to synchronize between QTKit- and AVFoundation-based elements.
Comment 7 Jer Noble 2011-11-02 15:02:14 PDT
(In reply to comment #5)
> Also, we prefer to have one patch per bug.  If you'd like to land these separately (which I think makes sense) please consider using multiple bugs.

Sure thing.  I'll update the title of this bug, obsolete the MediaController patch, and file new bugs for the subsequent patches.
Comment 8 Jer Noble 2011-11-02 15:08:57 PDT
Filed bug #71408 and bug #71410 and marked as depending on this bug.  I'll rebase my changes and get the updated.
Comment 9 Adam Barth 2011-11-02 15:21:23 PDT
Thanks.
Comment 10 Jer Noble 2011-11-02 16:34:22 PDT
Created attachment 113396 [details]
Source/JavaScriptCore: Implement MediaController.
Comment 11 Jer Noble 2011-11-02 16:34:38 PDT
Created attachment 113397 [details]
Source/WebCore: Implement MediaController.
Comment 12 Jer Noble 2011-11-02 16:38:29 PDT
Created attachment 113399 [details]
Patch

After a disasterous use of webkit-patch post-commit, webkit-patch upload is back to save the day...
Comment 13 Jer Noble 2011-11-02 16:40:01 PDT
Created attachment 113401 [details]
Patch
Comment 14 Jer Noble 2011-11-02 16:42:12 PDT
Sigh.  webkit-patch upload is still broken. (Adds the wrong patch to the wrong bug.)
Comment 15 Jer Noble 2011-11-02 16:47:27 PDT
<rdar://problem/10387316>
Comment 16 Adam Barth 2011-11-02 17:01:57 PDT
Comment on attachment 113399 [details]
Patch

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

PlatformClockPOSIX looks fine.  I'm not sure I fully understand the role of m_offset.  I don't know enough about CoreAudio to review that implementation.

> Source/WebCore/platform/posix/PlatformClockPOSIX.cpp:35
> +    return val.tv_sec + (val.tv_usec / 1000000.0f);

I would have named this constant, but I'm not sure that's a big deal.

> Source/WebCore/platform/posix/PlatformClockPOSIX.cpp:43
> +    if (delta.tv_usec < 0) {

Does this need to be a loop?  I'm not sure how much error checking you want to do here.
Comment 17 Jer Noble 2011-11-02 21:53:42 PDT
(In reply to comment #16)
> (From update of attachment 113399 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113399&action=review
> 
> PlatformClockPOSIX looks fine.  I'm not sure I fully understand the role of m_offset.  I don't know enough about CoreAudio to review that implementation.

Sure thing.  The general idea is that: current time = (now - start time) * rate + offset, and the offset gets reset whenever the clock is paused, played, or the rate changes.

> > Source/WebCore/platform/posix/PlatformClockPOSIX.cpp:35
> > +    return val.tv_sec + (val.tv_usec / 1000000.0f);
> 
> I would have named this constant, but I'm not sure that's a big deal.

I can add that. usecPerSec or similar.

> > Source/WebCore/platform/posix/PlatformClockPOSIX.cpp:43
> > +    if (delta.tv_usec < 0) {
> 
> Does this need to be a loop?  I'm not sure how much error checking you want to do here.

Unless something goes seriously wrong with gettimeofday, this will ever only need to be a single pass.  I could add an ASSERT(delta.tv_usec >= 0) after the if.
Comment 18 Jer Noble 2011-11-03 11:05:25 PDT
Committed r99222: <http://trac.webkit.org/changeset/99222>