Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
880873002:
WebVTT: Implement 'best position' from the rendering section (Closed)
|
Created:
5 years, 11 months ago by fs Modified:
5 years, 4 months ago Reviewers:
philipj_slow CC:
blink-reviews, nessy, zoltan1, eae+blinkwatch, gasubic, eric.carlson_apple.com, leviw+renderwatch, Dominik Röttsches, feature-media-reviews_chromium.org, blink-reviews-rendering, jchaffraix+rendering, pdr+renderingwatchlist_chromium.org, vcarbune.chromium Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionWebVTT: Implement 'best position' from the rendering section
This adds the spec. notion of 'best position', which brings the code in line
with the snap-to-lines part of the current draft.
BUG=301580
Patch Set 1 #Patch Set 2 : Fix compilation w/ asserts enabled. #
Total comments: 5
Patch Set 3 : Make computePositionScore impl. more clear. #
Messages
Total messages: 27 (1 generated)
fs@opera.com changed reviewers: + philipj@opera.com
https://codereview.chromium.org/880873002/diff/20001/LayoutTests/media/track/... File LayoutTests/media/track/track-cue-rendering-best-position.html (right): https://codereview.chromium.org/880873002/diff/20001/LayoutTests/media/track/... LayoutTests/media/track/track-cue-rendering-best-position.html:1: <!DOCTYPE html> If you know of any more "interesting" cases that would be worth testing let me know. (TBH, I'm not ecstatic about the change in behavior in this particular case...)
Some thoughts on the code, running the tests locally to see what I think of the results. https://codereview.chromium.org/880873002/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderVTTCue.cpp (right): https://codereview.chromium.org/880873002/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderVTTCue.cpp:253: LayoutRect intersectingBox = intersection(m_cueBox.containingBlock()->absoluteBoundingBoxRect(), boundingBox); Extract this into a LayoutRect titleAreaBox to be very explicit? Also makes the line shorter. https://codereview.chromium.org/880873002/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderVTTCue.cpp:256: return 1.0f - computeArea(intersectingBox) / computeArea(boundingBox); Can you assert that it's in the range [0,1]? The talk about percentages could lead one astray. Making it [0,100] would also be fine.
On 2015/01/27 10:14:52, fs wrote: > https://codereview.chromium.org/880873002/diff/20001/LayoutTests/media/track/... > File LayoutTests/media/track/track-cue-rendering-best-position.html (right): > > https://codereview.chromium.org/880873002/diff/20001/LayoutTests/media/track/... > LayoutTests/media/track/track-cue-rendering-best-position.html:1: <!DOCTYPE > html> > If you know of any more "interesting" cases that would be worth testing let me > know. (TBH, I'm not ecstatic about the change in behavior in this particular > case...) You mean you don't like what the spec says? You're probably the first to implement this, so should we change the spec?
Comparing the test results with and without this change I see what you mean. Now it will revert to the specified position if no non-overlapping position was found, even if that results in less readable text in total. Apparently this was in response to a spec bug I filed: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483 https://html5.org/r/7448 I'm not sure if we discussed this internally back then, if you think this is silly we can revisit it.
On 2015/01/27 10:36:57, philipj_UTC7 wrote: > On 2015/01/27 10:14:52, fs wrote: > > > https://codereview.chromium.org/880873002/diff/20001/LayoutTests/media/track/... > > File LayoutTests/media/track/track-cue-rendering-best-position.html (right): > > > > > https://codereview.chromium.org/880873002/diff/20001/LayoutTests/media/track/... > > LayoutTests/media/track/track-cue-rendering-best-position.html:1: <!DOCTYPE > > html> > > If you know of any more "interesting" cases that would be worth testing let me > > know. (TBH, I'm not ecstatic about the change in behavior in this particular > > case...) > > You mean you don't like what the spec says? You're probably the first to > implement this, so should we change the spec? Well, in this particular case we prefer to obscure the other cue, rather then overlap the video viewport ever so slightly. In real content I suppose we'll be pretty far into the weeds for this to actually happen - i.e. font-size is unlikely to be 120px for instance. It seems the new heuristic is more concerned with not overlapping the video viewport than anything else. The old heuristic was obviously a lot simpler, so in order to change it seems that some form of motivation/use case would've been involved (I don't remember/didn't follow the discussion). It'd probably be possible to have the heuristic pick the "old" (==specified) position in the tested case (by preferring the position where no other cues are overlapped), but it seems better to keep the heuristic as simple as possible TBH... https://codereview.chromium.org/880873002/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderVTTCue.cpp (right): https://codereview.chromium.org/880873002/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderVTTCue.cpp:253: LayoutRect intersectingBox = intersection(m_cueBox.containingBlock()->absoluteBoundingBoxRect(), boundingBox); On 2015/01/27 10:36:09, philipj_UTC7 wrote: > Extract this into a LayoutRect titleAreaBox to be very explicit? Also makes the > line shorter. Done. https://codereview.chromium.org/880873002/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderVTTCue.cpp:256: return 1.0f - computeArea(intersectingBox) / computeArea(boundingBox); On 2015/01/27 10:36:09, philipj_UTC7 wrote: > Can you assert that it's in the range [0,1]? The talk about percentages could > lead one astray. Making it [0,100] would also be fine. Modified this a bit to compute the area in LayoutUnit instead (and then remain in LayoutUnits until the division.)
On 2015/01/27 11:52:11, philipj_UTC7 wrote: > Comparing the test results with and without this change I see what you mean. Now > it will revert to the specified position if no non-overlapping position was > found, even if that results in less readable text in total. Apparently this was > in response to a spec bug I filed: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483 > https://html5.org/r/7448 > > I'm not sure if we discussed this internally back then, if you think this is > silly we can revisit it. Well, I suppose that trying to stay within the viewport makes some sense... It'd most likely be possible to create a TC that showed one heuristic poorer light compared to another, so it really boils down to what we want the heuristic to achieve. After saying that, I suppose I don't think this is _too_ silly - I was merely trying to come up with cases where the new one was (obviously/significantly) better than the old one (and mostly failing...)
I agree that simplicity is probably better than trying to do some marginally better thing in the case where the video is completely covered with text. I find myself agreeing with my former self in https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c1 "The strategy is to accept the first non-overlapping and completely contained position before switching and to accept the first non-overlapping position after switching." Would you prefer that, what the spec currently says, or something else?
On 2015/01/27 12:12:50, philipj_UTC7 wrote: > I agree that simplicity is probably better than trying to do some marginally > better thing in the case where the video is completely covered with text. I find > myself agreeing with my former self in > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c1 > > "The strategy is to accept the first non-overlapping and completely contained > position before switching and to accept the first non-overlapping position after > switching." > > Would you prefer that, what the spec currently says, or something else? Where "accept" ~= "best position" - or "done"? Sounds like it would prevent overlap in this case, but could also make cues that start out being out-of-bounds to end up like that as well. But we're talking severe edge-cases again here... If the algorithm/state requirements are smaller I'd prefer it ;-) (Which probably means that I prefer the old heuristic...)
On 2015/01/27 12:32:33, fs wrote: > On 2015/01/27 12:12:50, philipj_UTC7 wrote: > > I agree that simplicity is probably better than trying to do some marginally > > better thing in the case where the video is completely covered with text. I > find > > myself agreeing with my former self in > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c1 > > > > "The strategy is to accept the first non-overlapping and completely contained > > position before switching and to accept the first non-overlapping position > after > > switching." > > > > Would you prefer that, what the spec currently says, or something else? > > Where "accept" ~= "best position" - or "done"? Sounds like it would prevent > overlap in this case, but could also make cues that start out being > out-of-bounds to end up like that as well. But we're talking severe edge-cases > again here... If the algorithm/state requirements are smaller I'd prefer it ;-) > (Which probably means that I prefer the old heuristic...) I'm not sure what I meant in 2012, but now I think accept meaning "done positioning" makes sense, i.e. not storing any positions at all but simply trying to move the cue in both directions and accepting the first non-overlapping contained position, or just non-overlapping if no such position is going to be found by continuing to step. Do you think reverting https://html5.org/r/7448 on top of the current spec would be an improvement? If so I can try to prepare that change and see if it also matches my imaginary model above.
On 2015/01/27 15:51:44, philipj_UTC7 wrote: > On 2015/01/27 12:32:33, fs wrote: > > On 2015/01/27 12:12:50, philipj_UTC7 wrote: > > > I agree that simplicity is probably better than trying to do some marginally > > > better thing in the case where the video is completely covered with text. I > > find > > > myself agreeing with my former self in > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c1 > > > > > > "The strategy is to accept the first non-overlapping and completely > contained > > > position before switching and to accept the first non-overlapping position > > after > > > switching." > > > > > > Would you prefer that, what the spec currently says, or something else? > > > > Where "accept" ~= "best position" - or "done"? Sounds like it would prevent > > overlap in this case, but could also make cues that start out being > > out-of-bounds to end up like that as well. But we're talking severe edge-cases > > again here... If the algorithm/state requirements are smaller I'd prefer it > ;-) > > (Which probably means that I prefer the old heuristic...) > > I'm not sure what I meant in 2012, but now I think accept meaning "done > positioning" makes sense, i.e. not storing any positions at all but simply > trying to move the cue in both directions and accepting the first > non-overlapping contained position, or just non-overlapping if no such position > is going to be found by continuing to step. Hmm, so essentially push the cue out of view? (I assume that stepping would stop when the cue is no longer intersecting the viewport.) > Do you think reverting https://html5.org/r/7448 on top of the current spec would > be an improvement? If so I can try to prepare that change and see if it also > matches my imaginary model above. Can't really say with 100% certainty - it would (subjectively) "help" my TC at least... So it will probably be an improvement along some axis [1] - and it'll certainly be closer to what's described above. [1] I'm feeling a bit of a disconnect as to what axis is more interesting than the other.
Here's a simple revert: https://github.com/foolip/webvtt/compare/revert-best-position
That doesn't solve the problem "if the cue did not fit at all, it's moved back to its default position where it will overlap some other cue" from https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c0 though. Past me also said some useful things in https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c2
On 2015/01/28 11:25:57, philipj_UTC7 wrote: > That doesn't solve the problem "if the cue did not fit at all, it's moved back > to its default position where it will overlap some other cue" from > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c0 though. Past me also > said some useful things in > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c2 Maybe the past you intended to not restore to the default position if 'switched' is true.
I see that Mozilla have already implemented the "best position" bits: https://github.com/mozilla/vtt.js/blob/master/lib/vtt.js Perhaps a conservative fix would be to only store "best position" if that doesn't overlap any other cue and remove the boxes if there is no best position? Alternatively, rephrase this to find a single non-overlapping but possible not contained position in each direction. Pick the most contained one, or the first direction if they're equally good. Other ideas?
On 2015/01/28 11:38:48, fs wrote: > On 2015/01/28 11:25:57, philipj_UTC7 wrote: > > That doesn't solve the problem "if the cue did not fit at all, it's moved back > > to its default position where it will overlap some other cue" from > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c0 though. Past me also > > said some useful things in > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c2 > > Maybe the past you intended to not restore to the default position if 'switched' > is true. Yes, restoring either a default position or a best position is no good if it might be overlapping.
On 2015/01/28 11:51:59, philipj_UTC7 wrote: > I see that Mozilla have already implemented the "best position" bits: > https://github.com/mozilla/vtt.js/blob/master/lib/vtt.js Well, modulo bugs at least (I saw at least one =)). > Perhaps a conservative fix would be to only store "best position" if that > doesn't overlap any other cue and remove the boxes if there is no best position? Sounds reasonably simple to implement at least... (i.e. no additional state, some code movement.) > Alternatively, rephrase this to find a single non-overlapping but possible not > contained position in each direction. Pick the most contained one, or the first > direction if they're equally good. Probably fairly simple to implement too. > Other ideas? http://allthingsd.com/files/2011/11/double_facepalm.png and stay with the current version (given there's at least one existing implementation), and "bump to v2" (which could bring useful feedback [doubtful]).
On 2015/01/28 12:14:20, fs wrote: > On 2015/01/28 11:51:59, philipj_UTC7 wrote: > > I see that Mozilla have already implemented the "best position" bits: > > https://github.com/mozilla/vtt.js/blob/master/lib/vtt.js > > Well, modulo bugs at least (I saw at least one =)). File a bug and point to this discussion? Maybe Rick has strong opinions :) > > Perhaps a conservative fix would be to only store "best position" if that > > doesn't overlap any other cue and remove the boxes if there is no best > position? > > Sounds reasonably simple to implement at least... (i.e. no additional state, > some code movement.) Well, except the new state that is "best position". > > Alternatively, rephrase this to find a single non-overlapping but possible not > > contained position in each direction. Pick the most contained one, or the > first > > direction if they're equally good. > > Probably fairly simple to implement too. > > > Other ideas? > > http://allthingsd.com/files/2011/11/double_facepalm.png and stay with the > current version (given there's at least one existing implementation), and "bump > to v2" (which could bring useful feedback [doubtful]). Do you mean the current version of the spec or what is implemented here? How about Presto's fallback position described in https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c3 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c24 ? Looking at the Presto code it looks pretty simple to turn into spec and implementation if we wanted to. If renamed "best position" it could be a rather small spec change.
On 2015/01/28 16:50:58, philipj_UTC7 wrote: > On 2015/01/28 12:14:20, fs wrote: > > On 2015/01/28 11:51:59, philipj_UTC7 wrote: ... > > > Other ideas? > > > > http://allthingsd.com/files/2011/11/double_facepalm.png and stay with the > > current version (given there's at least one existing implementation), and > "bump > > to v2" (which could bring useful feedback [doubtful]). > > Do you mean the current version of the spec or what is implemented here? I _hope_ those are one and the same =) > How about Presto's fallback position described in > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c3 and > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c24 ? Looking at the Presto > code it looks pretty simple to turn into spec and implementation if we wanted > to. If renamed "best position" it could be a rather small spec change. I suppose it could be worth a shot (it does share a lot with the current spec. version).
On 2015/01/28 17:25:02, fs wrote: > On 2015/01/28 16:50:58, philipj_UTC7 wrote: > > On 2015/01/28 12:14:20, fs wrote: > > > On 2015/01/28 11:51:59, philipj_UTC7 wrote: > ... > > > > Other ideas? > > > > > > http://allthingsd.com/files/2011/11/double_facepalm.png and stay with the > > > current version (given there's at least one existing implementation), and > > "bump > > > to v2" (which could bring useful feedback [doubtful]). > > > > Do you mean the current version of the spec or what is implemented here? > > I _hope_ those are one and the same =) Uh, yes, I meant "currently implemented", not including this CL :) > > How about Presto's fallback position described in > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c3 and > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c24 ? Looking at the > Presto > > code it looks pretty simple to turn into spec and implementation if we wanted > > to. If renamed "best position" it could be a rather small spec change. > > I suppose it could be worth a shot (it does share a lot with the current spec. > version). How about this? https://github.com/foolip/webvtt/compare/better-position It's identical to what past me suggested except for the more complication conditions for switching direction. It's even more complicated than this CL, though.
On 2015/01/28 18:04:25, philipj_UTC7 wrote: > On 2015/01/28 17:25:02, fs wrote: > > On 2015/01/28 16:50:58, philipj_UTC7 wrote: > > > On 2015/01/28 12:14:20, fs wrote: > > > > On 2015/01/28 11:51:59, philipj_UTC7 wrote: > > ... > > > > > Other ideas? > > > > > > > > http://allthingsd.com/files/2011/11/double_facepalm.png and stay with the > > > > current version (given there's at least one existing implementation), and > > > "bump > > > > to v2" (which could bring useful feedback [doubtful]). > > > > > > Do you mean the current version of the spec or what is implemented here? > > > > I _hope_ those are one and the same =) > > Uh, yes, I meant "currently implemented", not including this CL :) Then I meant what this CL implements (==current version of the spec.) > > > How about Presto's fallback position described in > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c3 and > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c24 ? Looking at the > > Presto > > > code it looks pretty simple to turn into spec and implementation if we > wanted > > > to. If renamed "best position" it could be a rather small spec change. > > > > I suppose it could be worth a shot (it does share a lot with the current spec. > > version). > > How about this? https://github.com/foolip/webvtt/compare/better-position > > It's identical to what past me suggested except for the more complication > conditions for switching direction. It's even more complicated than this CL, > though. It does appear to require some changes to the switching logic, but that should be easy enough. (It's possible I missed something, since I always have a hard time reading documentation diffs...)
I realize now that I'd need to change "If there are any line boxes in the (possibly now repositioned) boxes that do not completely fit inside video's rendering area, remove those offending line boxes from boxes" for my change to do what I intended. https://github.com/foolip/webvtt/compare/better-position would use "best position" only to disambiguate between the non-contained positions straddling the top and bottom edge. Testing that seems like a bit of a nuisance and is perhaps needlessly perfect. Can you see a *simple* alternative that would mostly do the right thing and which you'd be happy to implement?
On 2015/01/29 10:15:42, philipj_UTC7 wrote: > I realize now that I'd need to change "If there are any line boxes in the > (possibly now repositioned) boxes that do not completely fit inside video's > rendering area, remove those offending line boxes from boxes" for my change to > do what I intended. I wouldn't want to implement that... > https://github.com/foolip/webvtt/compare/better-position would use "best > position" only to disambiguate between the non-contained positions straddling > the top and bottom edge. Testing that seems like a bit of a nuisance and is > perhaps needlessly perfect. > > Can you see a *simple* alternative that would mostly do the right thing and > which you'd be happy to implement? Nothing that's simpler than what has existed/exists (the "specified position"-only version is most likely the simplest - possibly excepting on that would just leave it be at the last tested position.) I'd be happy to try out the one in the diff above, I just need spend some more time deciphering it. I'm still a bit at a loss as to what "the right thing" is...
On 2015/01/29 11:33:20, fs wrote: > On 2015/01/29 10:15:42, philipj_UTC7 wrote: > > I realize now that I'd need to change "If there are any line boxes in the > > (possibly now repositioned) boxes that do not completely fit inside video's > > rendering area, remove those offending line boxes from boxes" for my change to > > do what I intended. > > I wouldn't want to implement that... The spec already says that, but it's not implemented. Doing that correctly together with a title safe area smaller than the video seems hard and not very important. Perhaps we should stop trying to do something clever when the cue doesn't fit and just drop it: https://github.com/foolip/webvtt/compare/simpler-position That's certainly simpler, or WDYT?
On 2015/01/29 15:49:24, philipj_UTC7 wrote: > On 2015/01/29 11:33:20, fs wrote: > > On 2015/01/29 10:15:42, philipj_UTC7 wrote: > > > I realize now that I'd need to change "If there are any line boxes in the > > > (possibly now repositioned) boxes that do not completely fit inside video's > > > rendering area, remove those offending line boxes from boxes" for my change > to > > > do what I intended. > > > > I wouldn't want to implement that... > > The spec already says that, but it's not implemented. Doing that correctly > together with a title safe area smaller than the video seems hard and not very > important. > > Perhaps we should stop trying to do something clever when the cue doesn't fit > and just drop it: > https://github.com/foolip/webvtt/compare/simpler-position > > That's certainly simpler, or WDYT? Yeah, I could/would support that. Less magic seems like a good thing.
OK, I've reopened the spec bug and opened a pull request: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c25 https://github.com/w3c/webvtt/pull/171
On 2015/01/29 17:15:09, philipj_UTC7 wrote: > OK, I've reopened the spec bug and opened a pull request: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c25 > https://github.com/w3c/webvtt/pull/171 This has now been merged, the 'best position' concept is no more. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
