Bug 71341 – Add Clock class and platform-specific implementations.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Implement MediaController.

[Jer Noble]

Created attachment 113339 [details]
Add Clock, PlatformClockCA, and PlatformClockPOSIX

[Jer Noble]

Created attachment 113364 [details]
MediaController and HTMLMediaElement changes.

[Adam Barth]

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.

[Adam Barth]

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?

[Adam Barth]

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.

[Jer Noble]

(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.

[Jer Noble]

(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.

[Jer Noble]

Filed bug #71408 and bug #71410 and marked as depending on this bug.  I'll rebase my changes and get the updated.

[Adam Barth]

Thanks.

[Jer Noble]

Created attachment 113396 [details]
Source/JavaScriptCore: Implement MediaController.

[Jer Noble]

Created attachment 113397 [details]
Source/WebCore: Implement MediaController.

[Jer Noble]

Created attachment 113399 [details]
Patch

After a disasterous use of webkit-patch post-commit, webkit-patch upload is back to save the day...

[Jer Noble]

Created attachment 113401 [details]
Patch

[Jer Noble]

Sigh.  webkit-patch upload is still broken. (Adds the wrong patch to the wrong bug.)

[Jer Noble]

<rdar://problem/10387316>

[Adam Barth]

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.

[Jer Noble]

(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.

[Jer Noble]

Committed r99222: <http://trac.webkit.org/changeset/99222>