Bug 79749 – Determine text direction for rendering a TextTrackCue

• Component: Media
• Status: RESOLVED FIXED

[Victor Carbune]

This bug represents substeps 10.2, 10.3 used to determine the text direction of the cue text.
http://dev.w3.org/html5/webvtt/#webvtt-cue-text-rendering-rules

"10.2 Apply the Unicode Bidirectional Algorithm's Paragraph Level steps to nodes using the following constraints, to determine the paragraph embedding level of the cue: [BIDI]
* nodes represents a single paragraph.
* The paragraph's text consists of the concatenation of the values of each WebVTT Text Object in nodes, in a pre-order, depth-first traversal, excluding WebVTT Ruby Text Objects and their descendants.

10.3 If the paragraph embedding level determined in the previous step is even (the paragraph direction is left-to-right), let direction be 'ltr', otherwise, let it be 'rtl'."

[Victor Carbune]

Created attachment 183639 [details]
WIP Patch

This is a work-in-progress patch, as I would like to make sure this is the right direction.

[Victor Carbune]

(In reply to comment #1)
> This is a work-in-progress patch, as I would like to make sure this is the right direction.

This is the way bidiResolver is called in GraphicsContext and TextRunIterator looks generic enough to me that it could be extracted from there and used in
both places.

[Eric Carlson]

Comment on attachment 183639 [details]
WIP Patch

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

> Source/WebCore/html/track/TextTrackCue.cpp:640
> +        if (!node->isTextNode() || node->localName() == rubyTag)

I believe a "WebVTT Ruby Text Object" is a node with a local name of "rt" (rtTag).

> Source/WebCore/html/track/TextTrackCue.cpp:648
> +    String paragraph = paragraphBuilder.toString();
> +    TextRun textRun(paragraph, paragraph.length());
> +    LOG(Media, "%s", paragraph.utf8().data());
> +

You can return early here if paragraphBuilder is empty.

[Victor Carbune]

Created attachment 184294 [details]
Patch to determine the text direction

[Victor Carbune]

(In reply to comment #3)
> (From update of attachment 183639 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183639&action=review
> 
> > Source/WebCore/html/track/TextTrackCue.cpp:640
> > +        if (!node->isTextNode() || node->localName() == rubyTag)
> 
> I believe a "WebVTT Ruby Text Object" is a node with a local name of "rt" (rtTag).
Done. I used the rtTag, but named badly the variable.
 
> > Source/WebCore/html/track/TextTrackCue.cpp:648
> > +    String paragraph = paragraphBuilder.toString();
> > +    TextRun textRun(paragraph, paragraph.length());
> > +    LOG(Media, "%s", paragraph.utf8().data());
> > +
> 
> You can return early here if paragraphBuilder is empty.
Done.

The patch is minimal to solely determine properly the text direction, since
while working on it I think the parameter determination for RTL cues
with is broken either in the implementation or in the spec.

I will file a different bug to extend the added test with multiple rendering
situations to make sure everything works properly (positioning/alignment/etc.)

[Victor Carbune]

Created attachment 184299 [details]
Rebased patch

[Eric Carlson]

Comment on attachment 184299 [details]
Rebased patch

This looks reasonable to me, but most of it is out of my area of expertise so I would rather that someone else r+ it.

[Victor Carbune]

CC-ing Eric in the hope

[Victor Carbune]

(In reply to comment #8)
> CC-ing Eric in the hope
In the hope of a review, I meant :-)

[Eric Seidel (no email)]

Bidi layout is normally not done in the DOM.  You need to know your line widths in order to paint.  I assume that these text tracks are never wrapped?

[Eric Seidel (no email)]

Comment on attachment 184299 [details]
Rebased patch

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

Can you help me understand what problem you're trying to solve?  This does not seem like the right solution.

> Source/WebCore/html/track/TextTrackCue.cpp:636
> +    // ... to determine the paragraph embedding level of the first Unicode paragraph of the cue.
> +
> +    // If the paragraph embedding level determined in the previous step is even
> +    // (the paragraph direction is left-to-right), let direction be 'ltr', otherwise, let it be 'rtl'.
> +    m_displayDirection = bidiRuns.firstRun()->level() % 2 ? CSSValueRtl : CSSValueLtr;

This is a huge amount of code to answer this very simple question.  You want basically text-direction: auto behavior it seems.

If this is the right way to do this (and I don't believe that), then this is the wrong abstraction to use.  You shouldn't have to do a full bidi line layout to ask what the direction of the first character is. :)

Also, Bi-di text is just that "bi directional", so it commonly has mixed LTR and RTL.  That's why we have lots of bidi runs.  They're not going to all be the same direction. :)

[Victor Carbune]

(In reply to comment #11)
> (From update of attachment 184299 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184299&action=review
> 
> Can you help me understand what problem you're trying to solve?  This does not seem like the right solution.

So the WebVTT Cue Text rendering rules depend on a direction parameter, which
is determined by the paragraph embedding level of the first paragraph of
the cue - which is the concatenation of all those WebVTT text nodes.

The direction parameter which results after the application of the Unicode
Bidirectional Algorithm is further used at establishing the following
parameters of a cue: x-position, y-position, maximum width (steps 10.4 - 10.11 of [1])

So this isn't a rendering of the cue lines (that's done later normally), it's
just a determination for parameters that place the overall cue box within
the video element (in case you think this is wrong, maybe we should file
some bugs on the WebVTT spec).

Either way, it looks like indeed what's needed is what
RenderBlockLineLayout::determineDirection() does, but I'm not sure if the
InlineIterator can be used here?


[1] http://dev.w3.org/html5/webvtt/#webvtt-cue-text-rendering-rules

[Levi Weintraub]

(In reply to comment #12)
> Either way, it looks like indeed what's needed is what
> RenderBlockLineLayout::determineDirection() does, but I'm not sure if the
> InlineIterator can be used here?

That was that conclusion I was afraid you'd come to. Since this isn't fundamentally rendered by the Rendering tree, we don't have Inlines for these boxes, right?

You're still likely doing more work than necessary in your determineTextDirection function. You could just be iterating across the characters looking for strong directionality.

[Victor Carbune]

(In reply to comment #13)
> (In reply to comment #12)
> > Either way, it looks like indeed what's needed is what
> > RenderBlockLineLayout::determineDirection() does, but I'm not sure if the
> > InlineIterator can be used here?
> 
> That was that conclusion I was afraid you'd come to. Since this isn't fundamentally rendered by the Rendering tree, we don't have Inlines for these boxes, right?

At the point where this algorithm is run, no rendering happens (this is a
preprocessing of the cue to ensure proper parameters are given when the actual
rendering is done).

> You're still likely doing more work than necessary in your determineTextDirection function. You could just be iterating across the characters looking for strong directionality.

This is precisely what I need to do! (and I thought that this is what I was
doing). So applying determineDirectionality with a TextRunIterator makes sense
to you?

[Levi Weintraub]

(In reply to comment #14)
> This is precisely what I need to do! (and I thought that this is what I was
> doing). So applying determineDirectionality with a TextRunIterator makes sense
> to you?

You're doing a lot more work than that! You're building a single string with all your text, creating text runs, and running the UBA over those text runs. All of this is necessary for rendering, but *not* for finding the directionality of the paragraph! You can simply iterate over the characters until you find one that's strongly directional. Then, obviously, bail.

[Aharon (Vladimir) Lanin]

Not sure if this is relevant to the current discussion, but the spec (http://dev.w3.org/html5/webvtt/#applying-css-properties-to-webvtt-node-objects) includes a full specification of the CSS to be used for a cue (and it is the CSS that is supposed to do all the rendering).

Bidi-wise, this includes:

the 'unicode-bidi' property must be set to 'plaintext'

the 'direction' property must be set to [the value] determined by the rules for updating the display of WebVTT text tracks for the text track cue from whose text the List of WebVTT Node Objects was constructed.

The 'text-align' property on the (root) List of WebVTT Node Objects must be set to the value in the second cell of the row of the table below whose first cell is the value of the corresponding cue's text track cue alignment:

Text track cue alignment	 'text-align' value
Start alignment	 'start'
Middle alignment	 'center'
End alignment	 'end'
Left alignment	 'left'
Right alignment	 'right'

[Victor Carbune]

(In reply to comment #16)
> Not sure if this is relevant to the current discussion, but the spec (http://dev.w3.org/html5/webvtt/#applying-css-properties-to-webvtt-node-objects) includes a full specification of the CSS to be used for a cue (and it is the CSS that is supposed to do all the rendering).

And that's what happens at that point - the cue is rendered with all the CSS
parameters applied (section 3.5.1 as you pointed out). This is really not about
rendering lines correctly, but computing the position of the cue within the
video (and after the cue block is positioned, lines are rendered inside it
normally).

This is about step 10.2 of section 3.5.

Let me give an example of the outcome for a cue with 'position' and 'alignment':

00:00:00.000 -> 00:00:30.000 position:30% align:end
Cue text (arabic or english)

If the direction determined in step 10.2 is:
* LTR, the parameters are:
size: 30%, x-position: 0%, y-position: 0%

* RTL, the parameters are:
size: 70%, x-position: 70%, y-position: 0%


So I'm just trying to plug in properly the direction parameter.


These parameters are then set as CSS values in:
Step 10.7: If the text track cue writing direction is horizontal, then let width be 'size vw'

Step 10.11: Let left be 'x-position vw' and top be 'y-position vh'. (These 
again are CSS values used by the next section to set CSS properties for the 
rendering; 'vw' and 'vh' are CSS units.)

[Victor Carbune]

Created attachment 187814 [details]
Simple iteration through characters

I stripped down the code to the simplest and direct method I could think of.

[Victor Carbune]

Ping about this? Are there concerns about the usefulness of the interpretation
of the positioning attributes in the case of RTL text?

[Levi Weintraub]

Comment on attachment 187814 [details]
Simple iteration through characters

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

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: media/track/track-cue-rendering-rtl.html

A general description of the patch would be nice.

> Source/WebCore/html/track/TextTrackCue.cpp:597
> +    // such as U+000A LINE FEED (LF), U+0085 NEXT LINE (NEL), and U+2029 PARAGRAPH SEPARATOR.
> +    return character == 0x000A || character == 0x0085 || character == 0x2029;

Is it "such as" or "specifically"? The spec says "only" from what I can tell.

> LayoutTests/media/track/track-cue-rendering-rtl-expected.txt:14
> +** The cue should contain arabic text **
> +EXPECTED (video.currentTime == '0.5') OK
> +EXPECTED (testTrack.track.activeCues.length == '1') OK
> +EXPECTED (testTrack.track.activeCues[0].text == 'تجربة') OK
> +EXPECTED (testCueDisplayBox.innerText == 'تجربة') OK
> +
> +** The position should be default and CSS direction set to RTL **
> +EXPECTED (2 * testCueDisplayBox.offsetLeft == video.videoWidth - testCueDisplayBox.offsetWidth == 'true') OK
> +EXPECTED (getComputedStyle(testCueDisplayBox).direction == 'rtl') OK

This test should check more permutations of text, like a cue with multiple paragraphs, neutral characters, etc.

[Levi Weintraub]

Other than the points I made, I think the implementation looks just fine.

[Victor Carbune]

Created attachment 193568 [details]
More test cases

[Victor Carbune]

(In reply to comment #20)
> (From update of attachment 187814 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187814&action=review
> 
> Is it "such as" or "specifically"? The spec says "only" from what I can tell.

I changed to check if Separator_Paragraph bit is set using Unicode::category.
I hope that's appropriate.

> This test should check more permutations of text, like a cue with multiple paragraphs, neutral characters, etc.

I added more variations with strong, weak and neutral characters.

Thanks for the review!

[Levi Weintraub]

Comment on attachment 193568 [details]
More test cases

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

Thanks, lgtm!

> Source/WebCore/html/track/TextTrackCue.cpp:591
> +    return WTF::Unicode::category(character) & WTF::Unicode::Separator_Paragraph;

FYI that this is an expensive, but functionally correct approach. Given the short size of the content usually being dealt with here, I think this is probably fine.

[Victor Carbune]

(In reply to comment #24)
> (From update of attachment 193568 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193568&action=review
> 
> Thanks, lgtm!
> 
> > Source/WebCore/html/track/TextTrackCue.cpp:591
> > +    return WTF::Unicode::category(character) & WTF::Unicode::Separator_Paragraph;
> 
> FYI that this is an expensive, but functionally correct approach. Given the short size of the content usually being dealt with here, I think this is probably fine.

Thanks, I'll keep this in mind (and change it, if there's a need) - at this 
point it should be run at most once per cue (or at most once each time the
cue contents change)

[WebKit Review Bot]

Comment on attachment 193568 [details]
More test cases

Clearing flags on attachment: 193568

Committed r146128: <http://trac.webkit.org/changeset/146128>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.