Bug 172640 – MediaTime class has rounding issues in different platforms

• Component: New Bugs
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

MediaTime class has rounding issues in different platforms

[Xabier Rodríguez Calvar]

Created attachment 311349 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

Please r?

[Darin Adler]

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.

[Xabier Rodríguez Calvar]

(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.

[Eric Carlson]

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

[Eric Carlson]

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.

[Darin Adler]

(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.

[Xabier Rodríguez Calvar]

(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?

[Darin Adler]

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.

[Jer Noble]

(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.

[Xabier Rodríguez Calvar]

Created attachment 312296 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 312296 [details]
Patch for landing

Clearing flags on attachment: 312296

Committed r217928: <http://trac.webkit.org/changeset/217928>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xabier Rodríguez Calvar]

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.

[Xabier Rodríguez Calvar]

(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.