Bug 190004 – WebVTT cue alignment broken

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Per Arne Vollan]

Closed caption rendering of proper cue alignment from a WebVTT file is broken. Left alignment is rendered on the right, right alignment is rendered on the left.

[Per Arne Vollan]

rdar://problem/32788929

[Per Arne Vollan]

Created attachment 350876 [details]
Patch

[Eric Carlson]

Comment on attachment 350876 [details]
Patch

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

> Source/WebCore/html/track/TextTrackCueGeneric.cpp:173
> +ExceptionOr<void> TextTrackCueGeneric::setPosition(const Variant<double, AutoKeyword>& position)

Nit: "Variant<double, AutoKeyword>" is used often enough that it would be good to define a type for it:  

using LineAndPositionSetting = Variant<double, AutoKeyword>;

> Source/WebCore/html/track/VTTCue.cpp:401
> +    Variant<double, AutoKeyword> pos;
> +    if (textPositionIsAuto())
> +        pos = Auto;
> +    else
> +        pos = m_textPosition;
> +    return pos;

Nit: this can be reduced to something like:

return textPositionIsAuto() ? Auto : m_textPosition;

> Source/WebCore/html/track/VTTCue.cpp:412
> +    if (position.index() == 1) {

I think WTF::holds_alternative<> would be cleaner: 

WTF::holds_alternative<AutoKeyword>(position)

[Per Arne Vollan]

(In reply to Eric Carlson from comment #3)
> Comment on attachment 350876 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350876&action=review
> 
> > Source/WebCore/html/track/TextTrackCueGeneric.cpp:173
> > +ExceptionOr<void> TextTrackCueGeneric::setPosition(const Variant<double, AutoKeyword>& position)
> 
> Nit: "Variant<double, AutoKeyword>" is used often enough that it would be
> good to define a type for it:  
> 
> using LineAndPositionSetting = Variant<double, AutoKeyword>;
> 
> > Source/WebCore/html/track/VTTCue.cpp:401
> > +    Variant<double, AutoKeyword> pos;
> > +    if (textPositionIsAuto())
> > +        pos = Auto;
> > +    else
> > +        pos = m_textPosition;
> > +    return pos;
> 
> Nit: this can be reduced to something like:
> 
> return textPositionIsAuto() ? Auto : m_textPosition;
> 
> > Source/WebCore/html/track/VTTCue.cpp:412
> > +    if (position.index() == 1) {
> 
> I think WTF::holds_alternative<> would be cleaner: 
> 
> WTF::holds_alternative<AutoKeyword>(position)

Thanks for reviewing! I will update the patch.

[Per Arne Vollan]

Created attachment 350883 [details]
Patch

[EWS Watchlist]

Comment on attachment 350883 [details]
Patch

Attachment 350883 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9359332

New failing tests:
media/track/track-webvtt-tc016-align-positioning.html
media/track/track-cue-mutable.html
media/track/track-vttcue.html
media/track/track-webvtt-tc021-valign.html
media/track/track-webvtt-tc018-align-text-line-position.html
media/track/track-add-remove-cue.html
media/track/track-webvtt-tc015-positioning.html
media/track/track-webvtt-tc013-settings.html

[EWS Watchlist]

Created attachment 350895 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 350883 [details]
Patch

Attachment 350883 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9359515

New failing tests:
media/track/track-webvtt-tc018-align-text-line-position.html
media/track/track-cue-mutable.html
media/track/track-vttcue.html
media/track/track-webvtt-tc021-valign.html
media/track/track-webvtt-tc016-align-positioning.html
media/track/track-add-remove-cue.html
media/track/track-webvtt-tc015-positioning.html
media/track/track-webvtt-tc013-settings.html

[EWS Watchlist]

Created attachment 350901 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Per Arne Vollan]

Created attachment 350905 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350905 [details]
Patch

Clearing flags on attachment: 350905

Committed r236531: <https://trac.webkit.org/changeset/236531>

[Truitt Savell]

Looks like the new test from https://trac.webkit.org/changeset/236531/webkit

media/track/track-cue-left-align.html

has been having image failures

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media%2Ftrack%2Ftrack-cue-left-align.html

Looks like it has a reference mismatch. 
https://trac.webkit.org/browser/trunk/LayoutTests/media/track/track-cue-left-align-expected-mismatch.html