Bug 109818 - Parsing WebVTT Region Header Metadata
Summary: Parsing WebVTT Region Header Metadata
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Victor Carbune
URL:
Keywords:
Depends on: 109820
Blocks: 109570
  Show dependency treegraph
 
Reported: 2013-02-14 05:44 PST by Victor Carbune
Modified: 2013-04-01 08:23 PDT (History)
16 users (show)

See Also:


Attachments
WIP Patch (17.33 KB, patch)
2013-02-14 06:26 PST, Victor Carbune
no flags Details | Formatted Diff | Diff
Updated patch (27.67 KB, patch)
2013-03-26 16:05 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Disabled regions (27.17 KB, patch)
2013-03-26 16:09 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Updated patch (28.75 KB, patch)
2013-04-01 04:07 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Proper guards (28.81 KB, patch)
2013-04-01 04:17 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Carbune 2013-02-14 05:44:29 PST
Support for parsing region metadata. For example:
"Region: id=fred width=50% height=3 regionanchor=0%,100% viewportanchor=10%,90% scroll=up"
Comment 1 Victor Carbune 2013-02-14 06:26:15 PST
Created attachment 188337 [details]
WIP Patch
Comment 2 Glenn Adams 2013-02-14 06:57:31 PST
where is this extension specified? it isn't in the current WebVTT spec [1]

[1] http://dev.w3.org/html5/webvtt/
Comment 3 Eric Carlson 2013-02-14 07:17:41 PST
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().
Comment 4 Eric Carlson 2013-02-14 07:28:50 PST
(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
Comment 5 Eric Carlson 2013-02-14 07:32:24 PST
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.
Comment 6 Silvia Pfeiffer 2013-03-19 00:55:49 PDT
FWIW: LGTM.
Comment 7 EFL EWS Bot 2013-03-19 01:02:33 PDT
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
Comment 8 Build Bot 2013-03-19 01:29:40 PDT
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
Comment 9 Build Bot 2013-03-19 01:30:37 PDT
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
Comment 10 Build Bot 2013-03-19 02:57:51 PDT
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
Comment 11 Victor Carbune 2013-03-26 16:05:00 PDT
Created attachment 195179 [details]
Updated patch
Comment 12 Victor Carbune 2013-03-26 16:09:36 PDT
Created attachment 195181 [details]
Disabled regions
Comment 13 Eric Carlson 2013-03-27 05:58:41 PDT
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.
Comment 14 Victor Carbune 2013-04-01 04:07:06 PDT
Created attachment 195950 [details]
Updated patch
Comment 15 EFL EWS Bot 2013-04-01 04:11:49 PDT
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
Comment 16 WebKit Review Bot 2013-04-01 04:12:41 PDT
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
Comment 17 WebKit Review Bot 2013-04-01 04:14:05 PDT
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
Comment 18 Victor Carbune 2013-04-01 04:17:28 PDT
Created attachment 195953 [details]
Proper guards
Comment 19 Victor Carbune 2013-04-01 04:36:51 PDT
(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).
Comment 20 WebKit Review Bot 2013-04-01 08:23:02 PDT
Comment on attachment 195953 [details]
Proper guards

Clearing flags on attachment: 195953

Committed r147325: <http://trac.webkit.org/changeset/147325>
Comment 21 WebKit Review Bot 2013-04-01 08:23:07 PDT
All reviewed patches have been landed.  Closing bug.