233901 – [VTT] Fix various issues with complicated rendering of VTT cues

Bug 233901 - [VTT] Fix various issues with complicated rendering of VTT cues

Summary: [VTT] Fix various issues with complicated rendering of VTT cues

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Media (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Jer Noble

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2021-12-06 16:15 PST by Jer Noble
Modified:	2021-12-09 10:38 PST (History)
CC List:	14 users (show)

See Also:	234090

Attachments
Patch (27.78 KB, patch) 2021-12-06 16:53 PST, Jer Noble	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch (29.03 KB, patch) 2021-12-06 17:02 PST, Jer Noble	no flags	Details \| Formatted Diff \| Diff
Patch (31.07 KB, patch) 2021-12-07 08:29 PST, Jer Noble	eric.carlson: review+ ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch for landing (46.67 KB, patch) 2021-12-07 16:01 PST, Jer Noble	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch for landing (46.85 KB, patch) 2021-12-07 16:46 PST, Jer Noble	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch for landing (46.86 KB, patch) 2021-12-07 23:07 PST, Jer Noble	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch for landing (47.07 KB, patch) 2021-12-08 10:46 PST, Jer Noble	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Jer Noble 2021-12-06 16:15:07 PST

[VTT] Fix various issues with complicated rendering of VTT cues

Comment 1 Jer Noble 2021-12-06 16:53:15 PST

Created attachment 446099 [details]
Patch

Comment 2 Jer Noble 2021-12-06 17:02:16 PST

Created attachment 446101 [details]
Patch

Comment 3 Jean-Yves Avenard [:jya] 2021-12-07 00:23:07 PST

Comment on attachment 446101 [details]
Patch

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

> Source/WebCore/html/track/VTTCue.h:248
> +    std::pair<float, float> m_displayPosition { undefinedPosition, undefinedPosition };

I think this should be std::optional<std::pair<...>> ; having particular values doing a particular behaviour is often a later source of error.

> Source/WebCore/rendering/RenderVTTCue.cpp:173
> +    return overlappingObjectForRect(backdropBox().absoluteBoundingBoxRect());

const RenderVTTCue*

> Source/WebCore/rendering/RenderVTTCue.cpp:359
>      if (downcast<TextTrackCueGeneric>(*m_cue).useDefaultPosition() && firstLineBox) {

could be testing for firstLineBox first

> Source/WebCore/rendering/RenderVTTCue.cpp:392
> +RenderBlockFlow& RenderVTTCue::backdropBox() const

const RenderBlockFlow&

> Source/WebCore/rendering/RenderVTTCue.h:53
> +    RenderVTTCue* overlappingObjectForRect(const IntRect&) const;

both should return `const RenderVTTCue*`

> Source/WebCore/rendering/RenderVTTCue.h:68
> +    RenderInline& cueBox() const;

this should return a const reference; particularly as the method is const.

Comment 4 Jer Noble 2021-12-07 08:29:39 PST

Created attachment 446176 [details]
Patch

Comment 5 Eric Carlson 2021-12-07 09:24:34 PST

Comment on attachment 446176 [details]
Patch

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

r=me

> Source/WebCore/rendering/RenderVTTCue.cpp:157
> +    return !rectIsWithinContainer(backdropBox().absoluteBoundingBoxRect());

It would be nice to have a convenience method on RenderVTTCue to replace all of the  `backdropBox().absoluteBoundingBoxRect()`.

> LayoutTests/media/track/track-webvtt-no-snap-to-lines-overlap.html:36
> +	<title>track-webvtt-no-snap-to-lines-overlap</title>
> +	<style>
> +		html { overflow:hidden }
> +		body { margin:0 }
> +		::cue {
> +		    font-family: Ahem, sans-serif;
> +		    font-size: 12px;
> +		    background-color: black;
> +		    color: green;
> +		}
> +	</style>
> +	<script>var requirePixelDump = true;</script>
> +	<script src="../video-test.js"></script>
> +	<script src="../media-file.js"></script>
> +	<script>
> +	window.addEventListener('load', async event => {
> +		findMediaElement();
> +
> +		run(`video.querySelector("track").src = "captions-webvtt/no-snap-to-lines-overlap.vtt"`);
> +		run(`video.textTracks[0].mode = "showing"`);
> +		await waitFor(video.querySelector('track'), 'load');
> +
> +		run(`video.src = findMediaFile("video", "w3c/track/webvtt/media/white")`);
> +		await waitFor(video, 'canplaythrough');
> +
> +		endTest();
> +	});
> +	</script>
> +</head>
> +<body>
> +	<video>
> +        <track id="testTrack" kind="captions" default>
> +	</video>

Mixed tabs and spaces!

Comment 6 Jer Noble 2021-12-07 15:51:54 PST

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> Comment on attachment 446101 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446101&action=review
> 
> > Source/WebCore/html/track/VTTCue.h:248
> > +    std::pair<float, float> m_displayPosition { undefinedPosition, undefinedPosition };
> 
> I think this should be std::optional<std::pair<...>> ; having particular
> values doing a particular behaviour is often a later source of error.

Agreed!

> > Source/WebCore/rendering/RenderVTTCue.cpp:173
> > +    return overlappingObjectForRect(backdropBox().absoluteBoundingBoxRect());
> 
> const RenderVTTCue*

I don't think that's necessary. The object returned doesn't belong to this RenderVTTCue at all.

> > Source/WebCore/rendering/RenderVTTCue.cpp:359
> >      if (downcast<TextTrackCueGeneric>(*m_cue).useDefaultPosition() && firstLineBox) {
> 
> could be testing for firstLineBox first

Ok.

> > Source/WebCore/rendering/RenderVTTCue.cpp:392
> > +RenderBlockFlow& RenderVTTCue::backdropBox() const
> 
> const RenderBlockFlow&

Since RenderElement::firstChild() is const and returns a non-const pointer, and this is just a wrapper around firstChild(), I don't actually think it's necessary here.

> > Source/WebCore/rendering/RenderVTTCue.h:53
> > +    RenderVTTCue* overlappingObjectForRect(const IntRect&) const;
> 
> both should return `const RenderVTTCue*`

Disagree for overlappingObjectForRect(); it's not owned by this RenderVTTCue object.

> > Source/WebCore/rendering/RenderVTTCue.h:68
> > +    RenderInline& cueBox() const;
> 
> this should return a const reference; particularly as the method is const.

Ditto for above.

Comment 7 Jer Noble 2021-12-07 16:01:07 PST

Created attachment 446246 [details]
Patch for landing

Comment 8 Jer Noble 2021-12-07 16:46:53 PST

Created attachment 446254 [details]
Patch for landing

Comment 9 Jer Noble 2021-12-07 23:07:23 PST

Created attachment 446312 [details]
Patch for landing

Comment 10 Jean-Yves Avenard [:jya] 2021-12-08 00:12:08 PST

Comment on attachment 446101 [details]
Patch

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

>>> Source/WebCore/rendering/RenderVTTCue.h:53
>>> +    RenderVTTCue* overlappingObjectForRect(const IntRect&) const;
>> 
>> both should return `const RenderVTTCue*`
> 
> Disagree for overlappingObjectForRect(); it's not owned by this RenderVTTCue object.

I don't think that this should prevent a pointer or reference from being made const.

It's not just about ownership IMHO, but also to indicate that this is a read-only object and that only read-only methods can be used with it.

Comment 11 Jer Noble 2021-12-08 10:46:33 PST

Created attachment 446385 [details]
Patch for landing

Comment 12 Jer Noble 2021-12-08 11:09:36 PST

(In reply to Jean-Yves Avenard [:jya] from comment #10)
> Comment on attachment 446101 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446101&action=review
> 
> >>> Source/WebCore/rendering/RenderVTTCue.h:53
> >>> +    RenderVTTCue* overlappingObjectForRect(const IntRect&) const;
> >> 
> >> both should return `const RenderVTTCue*`
> > 
> > Disagree for overlappingObjectForRect(); it's not owned by this RenderVTTCue object.
> 
> I don't think that this should prevent a pointer or reference from being
> made const.
> 
> It's not just about ownership IMHO, but also to indicate that this is a
> read-only object and that only read-only methods can be used with it.

It (the return value) is not a read-only object though.

Const isn't _necessarily_ transitive. All const says is that the call will not mutate the state of the underlying object. Policy can be made that const means more than that, but that's policy, not const, and WebKit does not have that policy, AFAIK.

Comment 13 EWS 2021-12-08 15:01:04 PST

Committed r286743 (244990@main): <https://commits.webkit.org/244990@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446385 [details].

Comment 14 Radar WebKit Bug Importer 2021-12-08 15:02:35 PST

<rdar://problem/86233716>