Bug 88787 – Support a rational time class for use by media elements.

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

[Jer Noble]

Support a rational time class for use by media elements.

[Jer Noble]

Created attachment 146868 [details]
Patch

First pass to allow EWS bots to chew on the new files.

[Gustavo Noronha (kov)]

Comment on attachment 146868 [details]
Patch

Attachment 146868 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12941364

[Early Warning System Bot]

Comment on attachment 146868 [details]
Patch

Attachment 146868 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12948184

[WebKit Review Bot]

Comment on attachment 146868 [details]
Patch

Attachment 146868 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12946199

[Early Warning System Bot]

Comment on attachment 146868 [details]
Patch

Attachment 146868 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12939412

[Jer Noble]

Created attachment 146870 [details]
Patch

Fix compile errors on mac, chromium and gkt ports.

[Eric Carlson]

Comment on attachment 146870 [details]
Patch

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

> Source/WTF/wtf/MediaTime.cpp:39
> +static int32_t greatestCommonDivisor(int32_t lhs, int32_t rhs)

What are "lhs" and "rhs" - "left hand side" and "right hand side"? In any case, please burn a few characters to make the variable names more obvious.

> Source/WTF/wtf/MediaTime.cpp:56
> +using namespace WTF;

Is this necessary inside of "namespace WTF { ..."?

> Source/WTF/wtf/MediaTime.cpp:291
> +    static const MediaTime* time = 0;
> +    if (!time)
> +        time = new MediaTime(0, 1, Valid);

This can be just "static const MediaTime* time = new MediaTime(...);"

Here and throughout the file.

[Jer Noble]

Created attachment 146872 [details]
Patch

Fixed the WTF::abs compile error.

[Jer Noble]

(In reply to comment #7)
> (From update of attachment 146870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146870&action=review
> 
> > Source/WTF/wtf/MediaTime.cpp:39
> > +static int32_t greatestCommonDivisor(int32_t lhs, int32_t rhs)
> 
> What are "lhs" and "rhs" - "left hand side" and "right hand side"? In any case, please burn a few characters to make the variable names more obvious.

I had thought rhs and lhs were pretty obvious. They're consistently used throughout WTF for similar functions.  This function isn't order-sensitive, so this could as easily be "int32_t a, int32_t b", or "int32_t one, int32_t two".  But the rest of the operators and constructors should be left as "lhs" and "rhs".

> > Source/WTF/wtf/MediaTime.cpp:56
> > +using namespace WTF;
> 
> Is this necessary inside of "namespace WTF { ..."?

Nope, removed.

> > Source/WTF/wtf/MediaTime.cpp:291
> > +    static const MediaTime* time = 0;
> > +    if (!time)
> > +        time = new MediaTime(0, 1, Valid);
> 
> This can be just "static const MediaTime* time = new MediaTime(...);"
> 
> Here and throughout the file.

Changed.

[Eric Carlson]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 146870 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146870&action=review
> > 
> > > Source/WTF/wtf/MediaTime.cpp:39
> > > +static int32_t greatestCommonDivisor(int32_t lhs, int32_t rhs)
> > 
> > What are "lhs" and "rhs" - "left hand side" and "right hand side"? In any case, please burn a few characters to make the variable names more obvious.
> 
> I had thought rhs and lhs were pretty obvious. They're consistently used throughout WTF for similar functions.  This function isn't order-sensitive, so this could as easily be "int32_t a, int32_t b", or "int32_t one, int32_t two".  But the rest of the operators and constructors should be left as "lhs" and "rhs".
> 
Oh well, it is just me then. Guess I should get out of the media ghetto more often.

[Jer Noble]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > (From update of attachment 146870 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=146870&action=review
> > > 
> > > > Source/WTF/wtf/MediaTime.cpp:39
> > > > +static int32_t greatestCommonDivisor(int32_t lhs, int32_t rhs)
> > > 
> > > What are "lhs" and "rhs" - "left hand side" and "right hand side"? In any case, please burn a few characters to make the variable names more obvious.
> > 
> > I had thought rhs and lhs were pretty obvious. They're consistently used throughout WTF for similar functions.  This function isn't order-sensitive, so this could as easily be "int32_t a, int32_t b", or "int32_t one, int32_t two".  But the rest of the operators and constructors should be left as "lhs" and "rhs".
> > 
> Oh well, it is just me then. Guess I should get out of the media ghetto more often.

No way, you knew what it meant on the first try! :)

[WebKit Review Bot]

Comment on attachment 146872 [details]
Patch

Attachment 146872 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12946213

[Jer Noble]

Created attachment 146881 [details]
Patch

Addressed Eric's comments and added ChangeLog entries.

[Eric Carlson]

Comment on attachment 146881 [details]
Patch

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

> Source/WTF/ChangeLog:73
> [phone]  Jer Noble  <[e-mail]>
> +
> +        Support a rational time class for use by media elements.
> +        https://bugs.webkit.org/show_bug.cgi?id=88787
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * GNUmakefile.list.am:

Oops, you forgot to nuke the old entry when you prettified it.

> Tools/ChangeLog:23
> [phone]  Jer Noble  <[e-mail]>
> +
> +        Support a rational time class for use by media elements.
> +        https://bugs.webkit.org/show_bug.cgi?id=88787
> +
> +        Reviewed by NOBODY (OOPS!).

Double-entry here too.

[Jer Noble]

Created attachment 146884 [details]
Patch

Add missing project files, including WTF.gypi and windows build files.

[Jer Noble]

(In reply to comment #14)
> (From update of attachment 146881 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146881&action=review
> 
> > Source/WTF/ChangeLog:73
> > [phone]  Jer Noble  <[e-mail]>
> > +
> > +        Support a rational time class for use by media elements.
> > +        https://bugs.webkit.org/show_bug.cgi?id=88787
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * GNUmakefile.list.am:
> 
> Oops, you forgot to nuke the old entry when you prettified it.
> 
> > Tools/ChangeLog:23
> > [phone]  Jer Noble  <[e-mail]>
> > +
> > +        Support a rational time class for use by media elements.
> > +        https://bugs.webkit.org/show_bug.cgi?id=88787
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> Double-entry here too.

Whoops!  Will fix that.

[Jer Noble]

Created attachment 146885 [details]
Patch

Removed double ChangeLog entries.

[Build Bot]

Comment on attachment 146885 [details]
Patch

Attachment 146885 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12940389

[Jer Noble]

Committed r123878: <http://trac.webkit.org/changeset/123878>

[Darin Adler]

Comment on attachment 146885 [details]
Patch

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

> Source/WTF/wtf/MediaTime.cpp:56
> +const int32_t MediaTime::MaximumTimeScale = 0x7fffffffL;

What is the L here for?

> Source/WTF/wtf/MediaTime.cpp:74
> +MediaTime::~MediaTime()
> +{
> +}

We should remove this explicit destructor. It does nothing, but incurs function call overhead. The compiler would do the right thing if we just didn’t declare or define it.

> Source/WTF/wtf/MediaTime.cpp:79
> +MediaTime::MediaTime(const MediaTime& rhs)
> +{
> +    *this = rhs;
> +}

We should remove this function. It just does what the compiler would already do if we didn’t write it, possibly less efficiently.

> Source/WTF/wtf/MediaTime.cpp:84
> +    if (floatTime != floatTime)
> +        return invalidTime();

We normally use isnan for this. Is there a reason not to do that here?

> Source/WTF/wtf/MediaTime.cpp:86
> +    if (std::isinf(floatTime))
> +        return std::signbit(floatTime) ? negativeInfiniteTime() : positiveInfiniteTime();

I’m surprised the std:: prefix is needed. Normally, if you include MathExtras.h it’s not, and including it can cause problems on some platforms.

> Source/WTF/wtf/MediaTime.cpp:100
> +    if (doubleTime != doubleTime)
> +        return invalidTime();

We normally use isnan for this. Is there a reason not to do that here?

> Source/WTF/wtf/MediaTime.cpp:102
> +    if (std::isinf(doubleTime))
> +        return std::signbit(doubleTime) ? negativeInfiniteTime() : positiveInfiniteTime();

I’m surprised the std:: prefix is needed. Normally, if you include MathExtras.h it’s not, and including it can cause problems on some platforms.

> Source/WTF/wtf/MediaTime.cpp:141
> +MediaTime& MediaTime::operator=(const MediaTime& rhs)
> +{
> +    m_timeValue = rhs.m_timeValue;
> +    m_timeScale = rhs.m_timeScale;
> +    m_timeFlags = rhs.m_timeFlags;
> +    return *this;
> +}

We should remove this function. It just does what the compiler would already do if we didn’t write it, possibly less efficiently.

> Source/WTF/wtf/MediaTime.cpp:240
> +bool MediaTime::operator<(const MediaTime& rhs) const
> +{
> +    return compare(rhs) == LessThan;
> +}
> +
> +bool MediaTime::operator>(const MediaTime& rhs) const
> +{
> +    return compare(rhs) == GreaterThan;
> +}
> +
> +bool MediaTime::operator==(const MediaTime& rhs) const
> +{
> +    return compare(rhs) == EqualTo;
> +}
> +
> +bool MediaTime::operator>=(const MediaTime& rhs) const
> +{
> +    return compare(rhs) >= EqualTo;
> +}
> +
> +bool MediaTime::operator<=(const MediaTime& rhs) const
> +{
> +    return compare(rhs) <= EqualTo;
> +}

These should be inline functions in the header.

> Source/WTF/wtf/MediaTime.cpp:313
> +const MediaTime& MediaTime::zeroTime()
> +{
> +    static const MediaTime* time = new MediaTime(0, 1, Valid);
> +    return *time;
> +}
> +
> +const MediaTime& MediaTime::invalidTime()
> +{
> +    static const MediaTime* time = new MediaTime(-1, 1, 0);
> +    return *time;
> +}
> +
> +const MediaTime& MediaTime::positiveInfiniteTime()
> +{
> +    static const MediaTime* time = new MediaTime(0, 1, PositiveInfinite | Valid);
> +    return *time;
> +}
> +
> +const MediaTime& MediaTime::negativeInfiniteTime()
> +{
> +    static const MediaTime* time = new MediaTime(-1, 1, NegativeInfinite | Valid);
> +    return *time;
> +}
> +
> +const MediaTime& MediaTime::indefiniteTime()
> +{
> +    static const MediaTime* time = new MediaTime(0, 1, Indefinite | Valid);
> +    return *time;
> +}

I see no reason for these to use pointers and put the objects on the heap.

> Source/WTF/wtf/MediaTime.cpp:334
> +static int32_t signum(int64_t val)

Is there a reason this function has to have this funny short non-word name? Is it some kind of standard function name? Maybe mathematicians know this name, and I don’t?

ALso, could we put this at the top of the file with the other pure math functions?

> Source/WTF/wtf/MediaTime.cpp:347
> +    MediaTime val = rhs;
> +    val.m_timeValue *= signum(rhs.m_timeScale) * signum(rhs.m_timeValue);
> +    return val;

Why abbreviate to “val”? I suggest using “value” instead.

> Source/WTF/wtf/MediaTime.h:64
> +    MediaTime& operator=(const MediaTime& rhs);
> +    MediaTime operator+(const MediaTime& rhs) const;
> +    MediaTime operator-(const MediaTime& rhs) const;
> +    bool operator<(const MediaTime& rhs) const;
> +    bool operator>(const MediaTime& rhs) const;
> +    bool operator==(const MediaTime& rhs) const;
> +    bool operator>=(const MediaTime& rhs) const;
> +    bool operator<=(const MediaTime& rhs) const;

Please leave out the “rhs” argument names in the header. Also, I’d prefer a word to an abbreviation for that argument name.

Also, the binary functions like == should be non-member functions so that the left side can be converted to a MediaTime, not just the right side. Fortunately, they don’t even have to be friend functions since they can just call the compare function.

> Source/WTF/wtf/MediaTime.h:70
> +    typedef enum {
> +        LessThan = -1,
> +        EqualTo = 0,
> +        GreaterThan = 1,
> +    } ComparisonFlags;

The preferred C++ syntax is enum Name {} rather than typedef enum {} Name. That’s also used above.

The name “Flags” doesn’t make sense to me for a type that is used for a value that is a single comparison result (not a flag or even multiple flags).

> Source/WTF/wtf/MediaTime.h:90
> +    friend MediaTime abs(const MediaTime& rhs);

Another way to do this would be to have a member function that gives the absolute value and then have abs just be an inline that calls that member function. Given the rest of the class’s interface is member functions, I think that would be more elegant than a friend.

> Source/WTF/wtf/MediaTime.h:92
> +    static const int32_t DefaultTimeScale = 6000;

What does this constant mean? What are its units? Why is 6000 a good value? Comments needed.

> Source/WTF/wtf/MediaTime.h:93
> +    static const int32_t MaximumTimeScale;

Why is the DefaultTimeScale value in the header, but not the MaximumTimeScale constant?

[Jer Noble]

(In reply to comment #20)
> (From update of attachment 146885 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146885&action=review

Whoops, i committed this patch this morning.  I'll address these comments in a new patch.

> > Source/WTF/wtf/MediaTime.cpp:56
> > +const int32_t MediaTime::MaximumTimeScale = 0x7fffffffL;
> 
> What is the L here for?
> 
> > Source/WTF/wtf/MediaTime.cpp:74
> > +MediaTime::~MediaTime()
> > +{
> > +}
> 
> We should remove this explicit destructor. It does nothing, but incurs function call overhead. The compiler would do the right thing if we just didn’t declare or define it.

Sure thing.

> > Source/WTF/wtf/MediaTime.cpp:79
> > +MediaTime::MediaTime(const MediaTime& rhs)
> > +{
> > +    *this = rhs;
> > +}
> 
> We should remove this function. It just does what the compiler would already do if we didn’t write it, possibly less efficiently.

Sure thing.

> > Source/WTF/wtf/MediaTime.cpp:84
> > +    if (floatTime != floatTime)
> > +        return invalidTime();
> 
> We normally use isnan for this. Is there a reason not to do that here?

Nope, no reason.

> > Source/WTF/wtf/MediaTime.cpp:86
> > +    if (std::isinf(floatTime))
> > +        return std::signbit(floatTime) ? negativeInfiniteTime() : positiveInfiniteTime();
> 
> I’m surprised the std:: prefix is needed. Normally, if you include MathExtras.h it’s not, and including it can cause problems on some platforms.

I'll pull it out; I see now where this could cause problems when including MathExtras.

> > Source/WTF/wtf/MediaTime.cpp:100
> > +    if (doubleTime != doubleTime)
> > +        return invalidTime();
> 
> We normally use isnan for this. Is there a reason not to do that here?

Nope.

> > Source/WTF/wtf/MediaTime.cpp:102
> > +    if (std::isinf(doubleTime))
> > +        return std::signbit(doubleTime) ? negativeInfiniteTime() : positiveInfiniteTime();
> 
> I’m surprised the std:: prefix is needed. Normally, if you include MathExtras.h it’s not, and including it can cause problems on some platforms.

Ditto.

> > Source/WTF/wtf/MediaTime.cpp:141
> > +MediaTime& MediaTime::operator=(const MediaTime& rhs)
> > +{
> > +    m_timeValue = rhs.m_timeValue;
> > +    m_timeScale = rhs.m_timeScale;
> > +    m_timeFlags = rhs.m_timeFlags;
> > +    return *this;
> > +}
> 
> We should remove this function. It just does what the compiler would already do if we didn’t write it, possibly less efficiently.

Sure.

> > Source/WTF/wtf/MediaTime.cpp:240
> > +bool MediaTime::operator<(const MediaTime& rhs) const
> > +{
> > +    return compare(rhs) == LessThan;
> > +}
> > +
> > +bool MediaTime::operator>(const MediaTime& rhs) const
> > +{
> > +    return compare(rhs) == GreaterThan;
> > +}
> > +
> > +bool MediaTime::operator==(const MediaTime& rhs) const
> > +{
> > +    return compare(rhs) == EqualTo;
> > +}
> > +
> > +bool MediaTime::operator>=(const MediaTime& rhs) const
> > +{
> > +    return compare(rhs) >= EqualTo;
> > +}
> > +
> > +bool MediaTime::operator<=(const MediaTime& rhs) const
> > +{
> > +    return compare(rhs) <= EqualTo;
> > +}
> 
> These should be inline functions in the header.

Sure.

> > Source/WTF/wtf/MediaTime.cpp:313
> > +const MediaTime& MediaTime::zeroTime()
> > +{
> > +    static const MediaTime* time = new MediaTime(0, 1, Valid);
> > +    return *time;
> > +}
> > +
> > +const MediaTime& MediaTime::invalidTime()
> > +{
> > +    static const MediaTime* time = new MediaTime(-1, 1, 0);
> > +    return *time;
> > +}
> > +
> > +const MediaTime& MediaTime::positiveInfiniteTime()
> > +{
> > +    static const MediaTime* time = new MediaTime(0, 1, PositiveInfinite | Valid);
> > +    return *time;
> > +}
> > +
> > +const MediaTime& MediaTime::negativeInfiniteTime()
> > +{
> > +    static const MediaTime* time = new MediaTime(-1, 1, NegativeInfinite | Valid);
> > +    return *time;
> > +}
> > +
> > +const MediaTime& MediaTime::indefiniteTime()
> > +{
> > +    static const MediaTime* time = new MediaTime(0, 1, Indefinite | Valid);
> > +    return *time;
> > +}
> 
> I see no reason for these to use pointers and put the objects on the heap.

Okay.

> > Source/WTF/wtf/MediaTime.cpp:334
> > +static int32_t signum(int64_t val)
> 
> Is there a reason this function has to have this funny short non-word name? Is it some kind of standard function name? Maybe mathematicians know this name, and I don’t?
> 
> ALso, could we put this at the top of the file with the other pure math functions?

Yes, it's a mathematician thing: http://en.wikipedia.org/wiki/Signum_function

And sure.

> > Source/WTF/wtf/MediaTime.cpp:347
> > +    MediaTime val = rhs;
> > +    val.m_timeValue *= signum(rhs.m_timeScale) * signum(rhs.m_timeValue);
> > +    return val;
> 
> Why abbreviate to “val”? I suggest using “value” instead.

Sure.

> > Source/WTF/wtf/MediaTime.h:64
> > +    MediaTime& operator=(const MediaTime& rhs);
> > +    MediaTime operator+(const MediaTime& rhs) const;
> > +    MediaTime operator-(const MediaTime& rhs) const;
> > +    bool operator<(const MediaTime& rhs) const;
> > +    bool operator>(const MediaTime& rhs) const;
> > +    bool operator==(const MediaTime& rhs) const;
> > +    bool operator>=(const MediaTime& rhs) const;
> > +    bool operator<=(const MediaTime& rhs) const;
> 
> Please leave out the “rhs” argument names in the header. Also, I’d prefer a word to an abbreviation for that argument name.
> 
> Also, the binary functions like == should be non-member functions so that the left side can be converted to a MediaTime, not just the right side. Fortunately, they don’t even have to be friend functions since they can just call the compare function.

Sure.

> > Source/WTF/wtf/MediaTime.h:70
> > +    typedef enum {
> > +        LessThan = -1,
> > +        EqualTo = 0,
> > +        GreaterThan = 1,
> > +    } ComparisonFlags;
> 
> The preferred C++ syntax is enum Name {} rather than typedef enum {} Name. That’s also used above.
> 
> The name “Flags” doesn’t make sense to me for a type that is used for a value that is a single comparison result (not a flag or even multiple flags).

ComparisonResult then.

> > Source/WTF/wtf/MediaTime.h:90
> > +    friend MediaTime abs(const MediaTime& rhs);
> 
> Another way to do this would be to have a member function that gives the absolute value and then have abs just be an inline that calls that member function. Given the rest of the class’s interface is member functions, I think that would be more elegant than a friend.

Sure.

> > Source/WTF/wtf/MediaTime.h:92
> > +    static const int32_t DefaultTimeScale = 6000;
> 
> What does this constant mean? What are its units? Why is 6000 a good value? Comments needed.

I'll add some.

> > Source/WTF/wtf/MediaTime.h:93
> > +    static const int32_t MaximumTimeScale;
> 
> Why is the DefaultTimeScale value in the header, but not the MaximumTimeScale constant?

Because, generally speaking, the DefaultTimeScale value is more interesting to the person using the API.  However, there's no strict reason why the MaximumTimeScale value couldn't be in the header either.

[Peter Kasting]

I landed a followon compile fix for Chromium/Win in r123968.  I had to move the _USE_MATH_DEFINES line; in doing so I copied the pattern from the places in WebCore that defined this macro, which both did it unconditionally above config.h.  Let me know if this causes problems.