Bug 109818 – Parsing WebVTT Region Header Metadata

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Victor Carbune]

Support for parsing region metadata. For example:
"Region: id=fred width=50% height=3 regionanchor=0%,100% viewportanchor=10%,90% scroll=up"

[Victor Carbune]

Created attachment 188337 [details]
WIP Patch

[Glenn Adams]

where is this extension specified? it isn't in the current WebVTT spec [1]

[1] http://dev.w3.org/html5/webvtt/

[Eric Carlson]

Comment on attachment 188337 [details]
WIP Patch

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

> Source/WebCore/html/track/TextTrackRegion.cpp:47
> +    : m_width(100)
> +    , m_height(3)
> +    , m_regionAnchor(std::make_pair(0, 100))
> +    , m_viewportAnchor(std::make_pair(0, 100))

I would prefer to see these come from named constants, with some information about why these values make sense.

> Source/WebCore/html/track/TextTrackRegion.cpp:89
> +    DEFINE_STATIC_LOCAL(const String, idKeyword, (ASCIILiteral("id")));
> +    DEFINE_STATIC_LOCAL(const String, heightKeyword, (ASCIILiteral("height")));
> +    DEFINE_STATIC_LOCAL(const String, widthKeyword, (ASCIILiteral("width")));
> +    DEFINE_STATIC_LOCAL(const String, regionAnchorKeyword, (ASCIILiteral("regionanchor")));
> +    DEFINE_STATIC_LOCAL(const String, viewportAnchorKeyword, (ASCIILiteral("viewportanchor")));
> +    DEFINE_STATIC_LOCAL(const String, scrollKeyword, (ASCIILiteral("scroll")));

These should probably be AtomicString.

> Source/WebCore/html/track/TextTrackRegion.cpp:109
> +    DEFINE_STATIC_LOCAL(const String, scrollUpValueKeyword, (ASCIILiteral("up")));

Ditto.

> Source/WebCore/html/track/TextTrackRegion.cpp:151
> +    case Id:
> +        if (value.find("-->") == notFound)
> +            m_id = value;
> +        break;
> +    case Width:
> +        number = WebVTTParser::parsePercentageValue(value, &isValidSetting);
> +        if (isValidSetting)
> +            m_width = number;
> +        break;
> +    case Height:
> +        position = 0;
> +
> +        numberAsString = WebVTTParser::collectDigits(value, &position);
> +        number = value.toInt(&isValidSetting);
> +
> +        if (isValidSetting && number >= 0)
> +            m_height = number;
> +        break;
> +    case RegionAnchor:
> +        anchorPosition = WebVTTParser::parsePercentageValuePair(value, ',', &isValidSetting);
> +        if (isValidSetting)
> +            m_regionAnchor = anchorPosition;
> +        break;
> +    case ViewportAnchor:
> +        anchorPosition = WebVTTParser::parsePercentageValuePair(value, ',', &isValidSetting);
> +        if (isValidSetting)
> +            m_viewportAnchor = anchorPosition;
> +        break;
> +    case Scroll:
> +        if (value == scrollUpValueKeyword)
> +            m_scroll = true;
> +        break;

Nit: I think it would be useful to LOG(Media, "...") when the parser encounters invalid input.

> Source/WebCore/html/track/WebVTTParser.cpp:101
> +    unsigned firstCoord = parsePercentageValue(
> +        value.substring(0, delimiterOffset),
> +        &isFirstValueValid);

Nit: I think the line breaks makes this more difficult to read.

> Source/WebCore/html/track/WebVTTParser.cpp:106
> +    unsigned secondCoord = parsePercentageValue(
> +        value.substring(delimiterOffset + 1, value.length() - 1),
> +        &isSecondValueValid);

Ditto.

> Source/WebCore/html/track/WebVTTParser.cpp:228
> +    if (!line.contains(":"))
> +        return;
> +
> +    unsigned colonPosition = line.find(":");

Nit: setting colonPosition first would mean you wouldn't also have to call line.contains().

[Eric Carlson]

(In reply to comment #2)
> where is this extension specified? it isn't in the current WebVTT spec [1]
> 
> [1] http://dev.w3.org/html5/webvtt/

As noted in the master bug (#109570):

Master bug for extending the WebVTT implementation with Regions:
https://dvcs.w3.org/hg/text-tracks/raw-file/default/608toVTT/region.html

Detailed feature explanation available here:
http://www.w3.org/community/texttracks/wiki/MultiCueBox

[Eric Carlson]

All of the code for this experimental feature should be inside an ENABLE() guard. "ENABLE(VIDEO_TRACK_REGIONS)" perhaps?

We should also discuss this on webkit-dev well before any changes are ready to commit.

[Silvia Pfeiffer]

FWIW: LGTM.

[EFL EWS Bot]

Comment on attachment 188337 [details]
WIP Patch

Attachment 188337 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17183554

[Build Bot]

Comment on attachment 188337 [details]
WIP Patch

Attachment 188337 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17161820

[Build Bot]

Comment on attachment 188337 [details]
WIP Patch

Attachment 188337 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17226232

[Build Bot]

Comment on attachment 188337 [details]
WIP Patch

Attachment 188337 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17187555

[Victor Carbune]

Created attachment 195179 [details]
Updated patch

[Victor Carbune]

Created attachment 195181 [details]
Disabled regions

[Eric Carlson]

Comment on attachment 195181 [details]
Disabled regions

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

> Source/WebCore/html/track/TextTrack.h:160
>      TextTrackRegionList* ensureTextTrackRegionList();

Nit: I think it would make more sense to leave this as a private implementation detail and expose a "regionList()" method.

> Source/WebCore/html/track/TextTrack.h:161
>      RefPtr<TextTrackRegionList> m_regions;

Can this stay private?

> Source/WebCore/html/track/TextTrackRegion.cpp:223
> +    DEFINE_STATIC_LOCAL(const String, idKeyword, (ASCIILiteral("id")));
> +    DEFINE_STATIC_LOCAL(const String, heightKeyword, (ASCIILiteral("height")));
> +    DEFINE_STATIC_LOCAL(const String, widthKeyword, (ASCIILiteral("width")));
> +    DEFINE_STATIC_LOCAL(const String, regionAnchorKeyword, (ASCIILiteral("regionanchor")));
> +    DEFINE_STATIC_LOCAL(const String, viewportAnchorKeyword, (ASCIILiteral("viewportanchor")));
> +    DEFINE_STATIC_LOCAL(const String, scrollKeyword, (ASCIILiteral("scroll")));

Making these AtomicString would be more efficient.

> Source/WebCore/html/track/TextTrackRegion.cpp:243
> +    DEFINE_STATIC_LOCAL(const String, scrollUpValueKeyword, (ASCIILiteral("up")));

Ditto.

> Source/WebCore/html/track/TextTrackRegion.cpp:246
> +

Nit: unnecessary blank space.

> Source/WebCore/html/track/TextTrackRegion.cpp:249
> +

Ditto.

> Source/WebCore/html/track/TextTrackRegion.cpp:261
> +        number = WebVTTParser::parseFloatPercentageValue(value, &isValidSetting);
> +        if (isValidSetting)
> +            m_width = number;

Nit: I know we don't do this in the WebVTT parser (yet), but I think it would be useful to LOG(Media, ...) when the parser encounters invalid input. This will make it easier to debug a class of "problem" later. Here and throughout this file.

> Source/WebCore/html/track/TextTrackRegion.cpp:265
> +

Ditto.

> Source/WebCore/html/track/WebVTTParser.cpp:70
> +float WebVTTParser::parseFloatPercentageValue(const String& value, bool* isValidSetting)

Nit: I think the bool parameter would be better as a reference instead of a pointer.

> Source/WebCore/html/track/WebVTTParser.cpp:97
> +FloatPoint WebVTTParser::parseFloatPercentageValuePair(const String& value, char delimiter, bool* isValidSetting)

Ditto.

> Source/WebCore/html/track/WebVTTParser.cpp:100
> +    size_t delimiterOffset = value.find(delimiter, 1);
> +    if (delimiterOffset == notFound || delimiterOffset == value.length() - 1 || !delimiterOffset) {

I think I see why it is illegal for the delimiter to be the second character, but it isn't immediately obvious so a comment would be helpful.

> Source/WebCore/html/track/WebVTTParser.cpp:108
> +    float firstCoord = parseFloatPercentageValue(
> +        value.substring(0, delimiterOffset),
> +        &isFirstValueValid);

Nit: I don't think the line breaks aid readability because the expression isn't all that wide.

> Source/WebCore/html/track/WebVTTParser.cpp:113
> +    float secondCoord = parseFloatPercentageValue(
> +        value.substring(delimiterOffset + 1, value.length() - 1),
> +        &isSecondValueValid);

Ditto.

> Source/WebCore/html/track/WebVTTParser.cpp:243
> +    DEFINE_STATIC_LOCAL(const String, regionHeaderName, (ASCIILiteral("Region")));

AtomicString

> Source/WebCore/html/track/WebVTTParser.cpp:395
> +    for (size_t i = 0; i < m_regionList.size(); ++i)
> +        if (m_regionList[i]->id() == region->id()) {
> +            m_regionList.remove(i);
> +            break;
> +        }

Does the spec say that the last ID encountered wins? In either case, does the logic to discard regions with duplicate IDs belong in the parser or elsewhere (I don't honestly know)?

> Source/WebCore/loader/TextTrackLoader.cpp:223
> +        LOG(Media, "Got %ld Regions!", outputRegions.size());

This is not an especially descriptive log line, it should at least include the class::method names.

[Victor Carbune]

Created attachment 195950 [details]
Updated patch

[EFL EWS Bot]

Comment on attachment 195950 [details]
Updated patch

Attachment 195950 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17394010

[WebKit Review Bot]

Comment on attachment 195950 [details]
Updated patch

Attachment 195950 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17246723

[WebKit Review Bot]

Comment on attachment 195950 [details]
Updated patch

Attachment 195950 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17346903

[Victor Carbune]

Created attachment 195953 [details]
Proper guards

[Victor Carbune]

(In reply to comment #13)
> (From update of attachment 195181 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195181&action=review
> 
> Does the spec say that the last ID encountered wins? In either case, does the logic to discard regions with duplicate IDs belong in the parser or elsewhere (I don't honestly know)?

I added as a comment the line which contains the drop instruction. Similar
approach happens for JS, just that it only overrides the parameters of existing
region (but it is uniform in the same that never two regions with the same
id end up in the same TextTrackRegionList).

> > Source/WebCore/loader/TextTrackLoader.cpp:223
> > +        LOG(Media, "Got %ld Regions!", outputRegions.size());
> 
> This is not an especially descriptive log line, it should at least include the class::method names.

Oups, removed this one.

Thanks for your review! (sorry for having to repeat some points from your review
on the WIP patch).

[WebKit Review Bot]

Comment on attachment 195953 [details]
Proper guards

Clearing flags on attachment: 195953

Committed r147325: <http://trac.webkit.org/changeset/147325>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.