Bug 65838 – Implement temporal dimension portion of Media Fragments URI specification for video/audio

• Component: Media
• Status: RESOLVED FIXED

[sideshowbarker]

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.

[sideshowbarker]

The related Mozilla bug is https://bugzilla.mozilla.org/show_bug.cgi?id=648595

[Deepak Sherveghar]

Hi,
Seems interesting. As I do want to implement it,  I'll look at what's involved.

[Eric Carlson]

<rdar://problem/10177041>

[Eric Carlson]

Created attachment 120832 [details]
Proposed patch

[Early Warning System Bot]

Comment on attachment 120832 [details]
Proposed patch

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

[Collabora GTK+ EWS bot]

Comment on attachment 120832 [details]
Proposed patch

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

[WebKit Review Bot]

Comment on attachment 120832 [details]
Proposed patch

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

[Eric Carlson]

Created attachment 120838 [details]
Updated patch

[Early Warning System Bot]

Comment on attachment 120838 [details]
Updated patch

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

[Eric Carlson]

Created attachment 120842 [details]
Updated patch

One more build system updated.

[Daniel Bates]

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?

[Eric Carlson]

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!

[Sam Weinig]

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.

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 121135 [details]
Updated patch

[Eric Carlson]

Created attachment 121180 [details]
Updated patch, one more time.

[Sam Weinig]

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.

[Eric Carlson]

http://trac.webkit.org/changeset/104197