Support a rational time class for use by media elements.
Created attachment 146868 [details] Patch First pass to allow EWS bots to chew on the new files.
Comment on attachment 146868 [details] Patch Attachment 146868 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12941364
Comment on attachment 146868 [details] Patch Attachment 146868 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12948184
Comment on attachment 146868 [details] Patch Attachment 146868 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12946199
Comment on attachment 146868 [details] Patch Attachment 146868 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12939412
Created attachment 146870 [details] Patch Fix compile errors on mac, chromium and gkt ports.
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.
Created attachment 146872 [details] Patch Fixed the WTF::abs compile error.
(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.
(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.
(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! :)
Comment on attachment 146872 [details] Patch Attachment 146872 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12946213
Created attachment 146881 [details] Patch Addressed Eric's comments and added ChangeLog entries.
Comment on attachment 146881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146881&action=review > Source/WTF/ChangeLog:73 > +2012-06-11 Jer Noble <jer.noble@apple.com> > + > + 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 > +2012-06-11 Jer Noble <jer.noble@apple.com> > + > + 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.
Created attachment 146884 [details] Patch Add missing project files, including WTF.gypi and windows build files.
(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 > > +2012-06-11 Jer Noble <jer.noble@apple.com> > > + > > + 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 > > +2012-06-11 Jer Noble <jer.noble@apple.com> > > + > > + 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.
Created attachment 146885 [details] Patch Removed double ChangeLog entries.
Comment on attachment 146885 [details] Patch Attachment 146885 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12940389
Committed r123878: <http://trac.webkit.org/changeset/123878>
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?
(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.
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.