Bug 233901

Summary:

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

Product:

WebKit

Reporter:

Jer Noble <jer.noble>

Component:

Media

Assignee:

Jer Noble <jer.noble>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jean-yves.avenard, kondapallykalyan, pdr, philipj, sergio, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=234090

Attachments:

Description	Flags
Patch	ews-feeder: commit-queue-
Patch	none
Patch	eric.carlson: review+, ews-feeder: commit-queue-
Patch for landing	ews-feeder: commit-queue-
Patch for landing	ews-feeder: commit-queue-
Patch for landing	ews-feeder: commit-queue-
Patch for landing	none

Jer Noble

Reported 2021-12-06 16:15:07 PST

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

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 attachment proposed patch, testcase, etc.

Jer Noble

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

Created attachment 446099 [details] Patch

Jer Noble

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

Created attachment 446101 [details] Patch

Jean-Yves Avenard [:jya]

Comment 3 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.

Jer Noble

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

Created attachment 446176 [details] Patch

Eric Carlson

Comment 5 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!

Jer Noble

Comment 6 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.

Jer Noble

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

Created attachment 446246 [details] Patch for landing

Jer Noble

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

Created attachment 446254 [details] Patch for landing

Jer Noble

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

Created attachment 446312 [details] Patch for landing

Jean-Yves Avenard [:jya]

Comment 10 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.

Jer Noble

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

Created attachment 446385 [details] Patch for landing

Jer Noble

Comment 12 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.

EWS

Comment 13 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].

Radar WebKit Bug Importer

Comment 14 2021-12-08 15:02:35 PST

<rdar://problem/86233716>

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