[webvtt] use actual cue box height as a step to adjust cue's position

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P2
normal
RESOLVED FIXED
3 months ago
24 days ago

People

(Reporter: alwu, Assigned: alwu)

Tracking

(Blocks 2 bugs)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

Assignee

Description

3 months ago

Now we're using font size as a step to adjust cue's position. However, it would cause accumulated error because there is a difference (< 1px) between the font size and the cue box's height.

In this bug, we should do

(1) use actual cue box's height as a positioning step
(2) get the correct line height when the cue text are wrapped into multiple line
(3) get the correct line height when applying CSS transformation

Assignee

Updated

3 months ago
Blocks: 1534862
Assignee

Comment 1

2 months ago

Now I'm trying to find the correct fuzzy threshold for those tests we will enable in this bug, because there are some weird white pixel color difference (difference <2 per channel) in the background.

Assignee

Comment 2

2 months ago

There is a tiny difference between font size and actual line box height, we should use actual line box's height or width to adjust cue's position in order to eliminate the positioning error.

As we use line's bounding box height, we should also consider the effect which CSS scale brings, because the bounding box which we get have already been applied transformation (eg. scaled).

Therefore, we would use the unscaled bounding box length to do the calculation.

In addition, when the text is wrapped into multiple lines due to the line box is not large enough to show all text in the single line, we would calculate what the single line box height is.

Assignee

Updated

2 months ago
Depends on: 1544661
Assignee

Comment 4

2 months ago

Sometime there would be some color rendering differences between the test and the reference, which is small enough to be ignored because human can't aware that difference.

Assignee

Comment 5

2 months ago

Now these two tests would permanently fail, not being timeout.

Assignee

Comment 6

2 months ago

There two tests should be permanently fail now.

Attachment #9059368 - Attachment is obsolete: true
Attachment #9059369 - Attachment is obsolete: true
Attachment #9059370 - Attachment is obsolete: true
Attachment #9057701 - Attachment description: Bug 1536762 - part2 : enable wpt tests. → Bug 1536762 - part2 : enable wpt tests and add 'fuzzy' comparision.
Assignee

Comment 7

2 months ago

There is a weird fail that the white video frame would be rendered to black, it's Android only fail. I've filed bug1546128 for investigation.

Assignee

Comment 8

2 months ago

There are two problems we have to do in this bug.

  1. Find correct height of the cue box
    As we are using font size as the cue box's height, but it has a difference from the real box height, which causes having a small positioning difference and resulted in the wpt ref test failures.

Therefore, we should use the actual box's size for positioning. There are three attributes we might use, offsetHeight, clientHeight and bounding box's height. It seems that we should use the bounding box's height, however, it's value would be affected by CSS transformation, we can only get the adjusted value.

Using adjusted value would cause incorrect result, so we would like to have a value which is not affected by any CSS tranformation. So we should consider offsetHeight or clientHeight. According to [1], clientHeight seems what we eager for.

However, during testing, I found that sometime the clientHeight is not equal to the actual box's height, there would have a tiny difference. After some investigate, I finally found that the difference is just because clientHeight we return is integer [2], not double!

After discussed with :heycam, I am going to add new chrome-only APIs to get the double version clientHeight and clientWidth.

  1. Get the single line height
    As cue box might contain multiple line, using the cue box's height directly as a basic positioning unit is wrong. We should find the single line and get its height.

The spec said [3]

Let step be the heigh or width of the first line box in boxes

We can use getClientRects() to get the independent line boxes, and then use the first box's height as a basic unit.

[1] https://developer.mozilla.org/en-US/docs/Web/API/CSS_Object_Model/Determining_the_dimensions_of_elements
[2] https://searchfox.org/mozilla-central/rev/f0ef51bfafc8ad0c3a2f523bf076edc57dc4891a/dom/base/Element.h#1277
[3] See step2, https://www.w3.org/TR/webvtt1/#ref-for-webvtt-cue-snap-to-lines-flag-12

Attachment #9059852 - Attachment description: Bug 1536762 - part3 : expected fail on Android. → Bug 1536762 - part3 : disable tests on Android and Windows.
Attachment #9057700 - Attachment description: Bug 1536762 - part1 : use acutal line box height as a basic positioning unit. → Bug 1536762 - part1 : use unscaled bounding box'size to adjust cue's posiition.
Attachment #9057700 - Attachment description: Bug 1536762 - part1 : use unscaled bounding box'size to adjust cue's posiition. → Bug 1536762 - part1 : use unscaled bounding box'size as a cue box's size.
Assignee

Comment 9

2 months ago

According to the spec [1] 7.2.10.2, we should use the first line box's height or width as positioning unit to adjust box's position.

We will also use this value to adjust box when snap-to-line is false.

There, we implement a new chrome-only API to acquire this information, which would return the first line box's size in the block frame.

[1] https://www.w3.org/TR/webvtt1/#ref-for-webvtt-cue-snap-to-lines-flag-12

Attachment #9057701 - Attachment description: Bug 1536762 - part2 : enable wpt tests and add 'fuzzy' comparision. → Bug 1536762 - part3 : enable wpt tests and add 'fuzzy' comparision.
Attachment #9059852 - Attachment description: Bug 1536762 - part3 : disable tests on Android and Windows. → Bug 1536762 - part4 : disable tests on Android and Windows.
Assignee

Comment 10

2 months ago

When adjusting box's position, we would choose a axis first and move the box along this axis to see whether we can place this box without overlapping with other boxes and fully inside the rendering area.

If the box has been over the boundary along the moving direction, we should move box back to the original position and change the moving direction to see whether we can find another best place for the box.

Although the adjustment can run without timeout now, it still doesn't match the result of the reference, so change the state to fail.

Assignee

Comment 11

Last month

In order to prevent causing any fail, push patches to try server after rebasing, and the result [1] looks good.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f16d7397781ca2f1448da2e9564525d94fca4a10&selectedJob=245605726

Comment 12

Last month
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d810269b72e
part1 : use unscaled bounding box'size as a cue box's size. r=baku,heycam
https://hg.mozilla.org/integration/autoland/rev/1fa720d18b38
part2 : use first line box's size as positioning basic unit. r=heycam,baku
https://hg.mozilla.org/integration/autoland/rev/9250cfd8c4e4
part3 : enable wpt tests and add 'fuzzy' comparision. r=heycam
https://hg.mozilla.org/integration/autoland/rev/f009e75ac871
part4 : disable tests on Android and Windows. r=heycam
https://hg.mozilla.org/integration/autoland/rev/f40e952d1552
part5 : adjustment should only happen when box doesn't go over the boundary along the moving direction. r=heycam
Assignee

Updated

24 days ago
Blocks: webvtt-wpt
No longer blocks: 1534862
You need to log in before you can comment on or make changes to this bug.