Bug 65838 - Implement temporal dimension portion of Media Fragments URI specification for video/audio
: Implement temporal dimension portion of Media Fragments URI specification for...
Status: RESOLVED FIXED
: WebKit
Media Elements
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Enhancement
Assigned To:
: http://www.w3.org/TR/media-frags/#nam...
:
:
:
  Show dependency treegraph
 
Reported: 2011-08-07 19:44 PST by
Modified: 2012-01-05 12:45 PST (History)


Attachments
Proposed patch (72.65 KB, patch)
2011-12-31 13:03 PST, Eric Carlson
webkit-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Updated patch (75.07 KB, patch)
2011-12-31 18:04 PST, Eric Carlson
webkit-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Updated patch (75.86 KB, patch)
2011-12-31 21:49 PST, Eric Carlson
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch (75.36 KB, patch)
2012-01-03 11:48 PST, Eric Carlson
sam: review-
Review Patch | Details | Formatted Diff | Diff
Updated patch (76.82 KB, patch)
2012-01-04 12:11 PST, Eric Carlson
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch, one more time. (76.34 KB, patch)
2012-01-04 15:45 PST, Eric Carlson
sam: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-08-07 19:44:07 PST
Chris Double is working on implementing the section of the Media Fragments URI specification that deals with temporal fragments for media resources. That section enables "clipping" of a video or audio resource so that only a certain range is played. The relevant part of the spec is here:

http://www.w3.org/TR/media-frags/#naming-time

It would be great to have an implementation in WebKit as well.
------- Comment #1 From 2011-08-07 19:48:44 PST -------
The related Mozilla bug is https://bugzilla.mozilla.org/show_bug.cgi?id=648595
------- Comment #2 From 2011-09-15 05:53:35 PST -------
Hi,
Seems interesting. As I do want to implement it,  I'll look at what's involved.
------- Comment #3 From 2011-12-31 12:01:53 PST -------
<rdar://problem/10177041>
------- Comment #4 From 2011-12-31 13:03:57 PST -------
Created an attachment (id=120832) [details]
Proposed patch
------- Comment #5 From 2011-12-31 13:12:35 PST -------
(From update of attachment 120832 [details])
Attachment 120832 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10997641
------- Comment #6 From 2011-12-31 13:24:42 PST -------
(From update of attachment 120832 [details])
Attachment 120832 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11028005
------- Comment #7 From 2011-12-31 13:27:29 PST -------
(From update of attachment 120832 [details])
Attachment 120832 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11024008
------- Comment #8 From 2011-12-31 18:04:02 PST -------
Created an attachment (id=120838) [details]
Updated patch
------- Comment #9 From 2011-12-31 18:11:13 PST -------
(From update of attachment 120838 [details])
Attachment 120838 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11032064
------- Comment #10 From 2011-12-31 21:49:29 PST -------
Created an attachment (id=120842) [details]
Updated patch

One more build system updated.
------- Comment #11 From 2012-01-03 11:32:14 PST -------
(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.

> 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?
------- Comment #12 From 2012-01-03 11:48:02 PST -------
Created an attachment (id=120974) [details]
Updated patch

(In reply to comment #11)
> (From update of attachment 120842 [details] [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 #13 From 2012-01-03 15:19:38 PST -------
(From update of attachment 120974 [details])
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 #14 From 2012-01-04 12:11:06 PST -------
(From update of attachment 120974 [details])
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.
------- Comment #15 From 2012-01-04 12:11:41 PST -------
Created an attachment (id=121135) [details]
Updated patch
------- Comment #16 From 2012-01-04 15:45:03 PST -------
Created an attachment (id=121180) [details]
Updated patch, one more time.
------- Comment #17 From 2012-01-05 11:17:57 PST -------
(From update of attachment 121180 [details])
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.
------- Comment #18 From 2012-01-05 12:45:34 PST -------
http://trac.webkit.org/changeset/104197