Summary: | Implement temporal dimension portion of Media Fragments URI specification for video/audio | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | sideshowbarker <mike> | ||||||||||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Enhancement | CC: | acolwell, bpwv64, dbates, dglazkov, eoconnor, eric.carlson, gustavo.noronha, gustavo, rakuco, silviapf, webkit.review.bot, xan.lopez | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
URL: | http://www.w3.org/TR/media-frags/#naming-time | ||||||||||||||||
Attachments: |
|
Description
sideshowbarker
2011-08-07 19:44:07 PDT
The related Mozilla bug is https://bugzilla.mozilla.org/show_bug.cgi?id=648595 Hi, Seems interesting. As I do want to implement it, I'll look at what's involved. Created attachment 120832 [details]
Proposed patch
Comment on attachment 120832 [details] Proposed patch Attachment 120832 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10997641 Comment on attachment 120832 [details] Proposed patch Attachment 120832 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11028005 Comment on attachment 120832 [details] Proposed patch Attachment 120832 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11024008 Created attachment 120838 [details]
Updated patch
Comment on attachment 120838 [details] Updated patch Attachment 120838 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11032064 Created attachment 120842 [details]
Updated patch
One more build system updated.
Comment on attachment 120842 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=120842&action=review I briefly looked at this patch and noticed some minor nits. I am not formally reviewing this patch. > Source/WebCore/html/MediaFragmentURIParser.cpp:14 > + * * Neither the name of Google Inc. nor the names of its Is this license block correct? Notice it references Google and it's a three-clause BSD license. The preferred license block for new files is <http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE>. That being said, you would know best as to which license block Apple uses. > Source/WebCore/html/MediaFragmentURIParser.cpp:38 > +#include "KURL.h" It's unnecessary to include this file since it's included by file MediaFragmentURIParser.h (above). > Source/WebCore/html/MediaFragmentURIParser.h:14 > + * * Neither the name of Google Inc. nor the names of its Is this license block correct? Notice it references Google and it's a three-clause BSD license. The preferred license block for new files is <http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE>. That being said, you would know best as to which license block Apple uses. > Source/WebCore/html/MediaFragmentURIParser.h:36 > +#include <KURL.h> Shouldn't this be written using double quotes-notation instead of angle-bracket notation? Created attachment 120974 [details] Updated patch (In reply to comment #11) > (From update of attachment 120842 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120842&action=review > > I briefly looked at this patch and noticed some minor nits. I am not formally reviewing this patch. > > > Source/WebCore/html/MediaFragmentURIParser.cpp:14 > > + * * Neither the name of Google Inc. nor the names of its > > Is this license block correct? Notice it references Google and it's a three-clause BSD license. The preferred license block for new files is <http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE>. That being said, you would know best as to which license block Apple uses. > Oops, fixed! > > Source/WebCore/html/MediaFragmentURIParser.cpp:38 > > +#include "KURL.h" > > It's unnecessary to include this file since it's included by file MediaFragmentURIParser.h (above). > Thanks, fixed. > > Source/WebCore/html/MediaFragmentURIParser.h:14 > > + * * Neither the name of Google Inc. nor the names of its > > Is this license block correct? Notice it references Google and it's a three-clause BSD license. The preferred license block for new files is <http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE>. That being said, you would know best as to which license block Apple uses. > Fixed. > > Source/WebCore/html/MediaFragmentURIParser.h:36 > > +#include <KURL.h> > > Shouldn't this be written using double quotes-notation instead of angle-bracket notation? > Fixed. Thanks for taking a look! Comment on attachment 120974 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=120974&action=review > Source/WebCore/html/HTMLMediaElement.cpp:3693 > + OwnPtr<MediaFragmentURIParser> fragmentParser = MediaFragmentURIParser::create(m_currentSrc); Can we just allocate the parser on the stack instead of allocating it? > Source/WebCore/html/HTMLMediaElement.cpp:3697 > + float dur = duration(); Why is this a float and everything else a double? > Source/WebCore/html/HTMLMediaElement.cpp:3704 > + m_fragmentStartTime = (start > 0) ? start : MediaFragmentURIParser::invalidTimeValue(); > + if (m_fragmentStartTime > dur) > + m_fragmentStartTime = dur; > + > + m_fragmentEndTime = (end > 0 && end > start) ? end : MediaFragmentURIParser::invalidTimeValue(); > + if (m_fragmentEndTime > dur) > + m_fragmentEndTime = dur; It seems like this code is making the assumption about what MediaFragmentURIParser::invalidTimeValue() is. It seems like you could restructure it so the following if-statements only happen if you are not assigning MediaFragmentURIParser::invalidTimeValue(). > Source/WebCore/html/MediaFragmentURIParser.cpp:50 > +static void skipWhiteSpace(const String& input, unsigned& position) > +{ > + while (position < input.length() && WTF::isASCIISpace(input[position])) > + ++position; I find for functions like this, it is good to either quote the spec, or at least link to it, to explain, for instance, why isASCIISpace is the right whitespace function. I also believe Micheal has been trying to get people off of iterating Strings directly, and instead using the characters8() or characters16() functions, but I will ask him to verify that. > Source/WebCore/html/MediaFragmentURIParser.cpp:58 > + StringBuilder digits; > + while (position < input.length() && isASCIIDigit(input[position])) > + digits.append(input[position++]); > + return digits.toString(); Same comment as above. > Source/WebCore/html/MediaFragmentURIParser.cpp:70 > + StringBuilder digits; > + if (input[position] != '.') > + return String(); > + > + digits.append(input[position++]); > + while (position < input.length() && isASCIIDigit(input[position])) > + digits.append(input[position++]); > + return digits.toString(); Same comment as above. > Source/WebCore/html/MediaFragmentURIParser.cpp:192 > + ASSERT(m_timeFormat == None); > + > + if (m_fragments.isEmpty()) > + parseFragments(); > + > + double start; > + double end; > + > + for (unsigned i = 0; i < m_fragments.size(); ++i) { > + pair<String, String>& fragment = m_fragments[i]; > + > + // http://www.w3.org/2008/WebVideo/Fragments/WD-media-fragments-spec/#naming-time > + // Temporal clipping is denoted by the name t, and specified as an interval with a begin > + // time and an end time > + if (fragment.first != "t") > + continue; > + > + if (parseNPTFragment(fragment.second, start, end)) { > + m_startTime = start; > + m_endTime = end; > + m_timeFormat = NormalPlayTime; > + > + // Although we have a valid fragment, don't return yet because when a fragment > + // dimensions occurs multiple times, only the last occurrence of that dimension > + // is used: > + // http://www.w3.org/2008/WebVideo/Fragments/WD-media-fragments-spec/#error-uri-general > + // Multiple occurrences of the same dimension: only the last valid occurrence of a dimension > + // (e.g., t=10 in #t=2&t=10) is interpreted, all previous occurrences (valid or invalid) > + // SHOULD be ignored by the UA. > + } > + } Could you kill the fragment vector once you have finished parsing? > Source/WebCore/html/MediaFragmentURIParser.cpp:209 > + if (timeString.substring(0, nptIdentiferLength) == "npt:") > + offset += nptIdentiferLength; You could probably do this more efficiently since substring allocates a String. I would recommend just looking at the characters. > Source/WebCore/html/MediaFragmentURIParser.cpp:331 > +} > + > + > +} > + > +#endif Got some extra whitespace here. > Source/WebCore/html/MediaFragmentURIParser.h:35 > +#include <wtf/text/StringBuilder.h> > +#include <wtf/text/StringHash.h> You don't seem to use these here. > Source/WebCore/html/MediaFragmentURIParser.h:43 > + virtual ~MediaFragmentURIParser() { } You don't have any virtual functions or reason to subclass this, so you could probably remove this destructor and mark the class FINAL. Comment on attachment 120974 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=120974&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:3693 >> + OwnPtr<MediaFragmentURIParser> fragmentParser = MediaFragmentURIParser::create(m_currentSrc); > > Can we just allocate the parser on the stack instead of allocating it? Done. >> Source/WebCore/html/HTMLMediaElement.cpp:3704 >> + m_fragmentEndTime = dur; > > It seems like this code is making the assumption about what MediaFragmentURIParser::invalidTimeValue() is. It seems like you could restructure it so the following if-statements only happen if you are not assigning MediaFragmentURIParser::invalidTimeValue(). Done. >> Source/WebCore/html/MediaFragmentURIParser.cpp:50 >> + ++position; > > I find for functions like this, it is good to either quote the spec, or at least link to it, to explain, for instance, why isASCIISpace is the right whitespace function. I also believe Micheal has been trying to get people off of iterating Strings directly, and instead using the characters8() or characters16() functions, but I will ask him to verify that. Good suggestion. Re-reading the spec I see that whitespace isn't allowed at all so the function is gone. The fragment string is known to be utf8 by the time it is processed, so I changed the parsing functions to operate on a const LChar* and a length. >> Source/WebCore/html/MediaFragmentURIParser.cpp:58 >> + return digits.toString(); > > Same comment as above. Done. >> Source/WebCore/html/MediaFragmentURIParser.cpp:70 >> + return digits.toString(); > > Same comment as above. Done. >> Source/WebCore/html/MediaFragmentURIParser.cpp:192 >> + } > > Could you kill the fragment vector once you have finished parsing? Done. >> Source/WebCore/html/MediaFragmentURIParser.cpp:209 >> + offset += nptIdentiferLength; > > You could probably do this more efficiently since substring allocates a String. I would recommend just looking at the characters. Done. >> Source/WebCore/html/MediaFragmentURIParser.cpp:331 >> +#endif > > Got some extra whitespace here. Gone. >> Source/WebCore/html/MediaFragmentURIParser.h:35 >> +#include <wtf/text/StringHash.h> > > You don't seem to use these here. Nope, gone. >> Source/WebCore/html/MediaFragmentURIParser.h:43 >> + virtual ~MediaFragmentURIParser() { } > > You don't have any virtual functions or reason to subclass this, so you could probably remove this destructor and mark the class FINAL. Done. Created attachment 121135 [details]
Updated patch
Created attachment 121180 [details]
Updated patch, one more time.
Comment on attachment 121180 [details] Updated patch, one more time. View in context: https://bugs.webkit.org/attachment.cgi?id=121180&action=review > Source/WebCore/html/MediaFragmentURIParser.h:45 > + static MediaFragmentURIParser create(const KURL& url) > + { > + return MediaFragmentURIParser(url); > + } You don't need this create function, you can just make the constructor public. |