MediaTime class has rounding issues in different platforms
Created attachment 311349 [details] Patch
Comment on attachment 311349 [details] Patch Attachment 311349 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3821463 New failing tests: fast/events/before-unload-returnValue.html fast/css/target-fragment-match.html
Created attachment 311350 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
(In reply to Build Bot from comment #2) > New failing tests: > fast/events/before-unload-returnValue.html > fast/css/target-fragment-match.html These seem unrelated.
Please r?
Comment on attachment 311349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311349&action=review > Source/WTF/wtf/MediaTime.cpp:148 > - return MediaTime(static_cast<int64_t>(doubleTime * timeScale), timeScale, Valid); > + return MediaTime(static_cast<int64_t>(std::round(doubleTime * timeScale)), timeScale, Valid); I would have expected the old code to truncate, not round, on all platforms. Is rounding what we want here? > Source/WTF/wtf/MediaTime.cpp:558 > void MediaTime::dump(PrintStream &out) const Incorrect formatting of PrintStream& here.
(In reply to Darin Adler from comment #6) > Comment on attachment 311349 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=311349&action=review > > > Source/WTF/wtf/MediaTime.cpp:148 > > - return MediaTime(static_cast<int64_t>(doubleTime * timeScale), timeScale, Valid); > > + return MediaTime(static_cast<int64_t>(std::round(doubleTime * timeScale)), timeScale, Valid); > > I would have expected the old code to truncate, not round, on all platforms. > Is rounding what we want here? I think rounding is usually better, also in this case > > Source/WTF/wtf/MediaTime.cpp:558 > > void MediaTime::dump(PrintStream &out) const > > Incorrect formatting of PrintStream& here. Wdym? I changed it to be like that cause I think it is more informative. In case the stored value is a double we just print that cause the fraction is noise (literally, the double interpreted as integer because it's a union) and in case it is a fraction we print it and as informative value, its representation as double.
Comment on attachment 311349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311349&action=review >>> Source/WTF/wtf/MediaTime.cpp:558 >>> void MediaTime::dump(PrintStream &out) const >> >> Incorrect formatting of PrintStream& here. > > Wdym? I changed it to be like that cause I think it is more informative. In case the stored value is a double we just print that cause the fraction is noise (literally, the double interpreted as integer because it's a union) and in case it is a fraction we print it and as informative value, its representation as double. I think Darin means the "&" should be next to the type, not the variable: void MediaTime::dump(PrintStream& out) const
Comment on attachment 311349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311349&action=review >>>> Source/WTF/wtf/MediaTime.cpp:558 >>>> void MediaTime::dump(PrintStream &out) const >>> >>> Incorrect formatting of PrintStream& here. >> >> Wdym? I changed it to be like that cause I think it is more informative. In case the stored value is a double we just print that cause the fraction is noise (literally, the double interpreted as integer because it's a union) and in case it is a fraction we print it and as informative value, its representation as double. > > I think Darin means the "&" should be next to the type, not the variable: > > void MediaTime::dump(PrintStream& out) const ... because that is the preferred WebKit style.
(In reply to Eric Carlson from comment #8) > I think Darin means the "&" should be next to the type, not the variable: > > void MediaTime::dump(PrintStream& out) const (In reply to Eric Carlson from comment #9) > ... because that is the preferred WebKit style. Yes, right, that’s what I meant. Sorry my wording was unclear.
(In reply to Darin Adler from comment #10) > (In reply to Eric Carlson from comment #8) > > I think Darin means the "&" should be next to the type, not the variable: > > > > void MediaTime::dump(PrintStream& out) const > > (In reply to Eric Carlson from comment #9) > > ... because that is the preferred WebKit style. > > Yes, right, that’s what I meant. Sorry my wording was unclear. Sure! I hadn't even noticed but because that was not part of the patch, but I can change that. Might I have and r+ cq- so that I can land with that change?
Sure, but I think it should be someone who works on the media code and has more expertise than I. I didn’t review because I’m not familiar enough with all the uses of MediaTime to be sure that the change to rounding is safe and helpful. I suspect it is, but I would like someone else to make the call.
(In reply to Darin Adler from comment #12) > Sure, but I think it should be someone who works on the media code and has > more expertise than I. I didn’t review because I’m not familiar enough with > all the uses of MediaTime to be sure that the change to rounding is safe and > helpful. I suspect it is, but I would like someone else to make the call. Looks fine to me.
Created attachment 312296 [details] Patch for landing
Comment on attachment 312296 [details] Patch for landing Clearing flags on attachment: 312296 Committed r217928: <http://trac.webkit.org/changeset/217928>
All reviewed patches have been landed. Closing bug.
I am unsure about: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=media%2Fmodern-media-controls%2Fbuttons-container%2Fbuttons-container-buttons-property.html&revision=217791 https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=media%2Fvideo-seek-with-negative-playback.html I'll check it back tomorrow but if you're sure it's a problem, we could roll back if urgent.
(In reply to Xabier Rodríguez Calvar from comment #17) > I am unsure about: > > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#tests=media%2Fmodern-media-controls%2Fbuttons-container%2Fbuttons- > container-buttons-property.html&revision=217791 > > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#tests=media%2Fvideo-seek-with-negative-playback.html Flakiness dashboard says they are ok.