Fix rounding errors in MediaDecoderStateMachine::HaveEnoughDecodedAudio()

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
http://searchfox.org/mozilla-central/rev/22be34bcc4d5c56b62482a537bba77a6cdce117b/dom/media/MediaDecoderStateMachine.cpp#2502

Say the result of |mAmpleAudioThresholdUsecs * mPlaybackRate| is 10.5, it will be rounded down to 10 when assigned to |ampleAudioUSecs|.

HaveEnoughDecodedAudio() will return true if GetDecodedAudioDuration() is 10. However, it should return false because GetDecodedAudioDuration() == 10 < 10.5.
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
Blocks: 1324999
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8823931 [details]
Bug 1328781. part 1 - somewhat rewrite the expression.

https://reviewboard.mozilla.org/r/102392/#review102788
Attachment #8823931 - Flags: review?(kikuo) → review+

Comment 4

2 years ago
mozreview-review
Comment on attachment 8823932 [details]
Bug 1328781. part 2 - don't convert a double to an int64_t to avoid rounding errors.

https://reviewboard.mozilla.org/r/102394/#review102792

::: dom/media/MediaDecoderStateMachine.cpp:2506
(Diff revision 1)
> -bool MediaDecoderStateMachine::HaveEnoughDecodedAudio()
> +bool
> +MediaDecoderStateMachine::HaveEnoughDecodedAudio()

It turns out that the style of this call and and the following |MDSM::HaveEnoughDecodedVideo| are the minority around here.

IMHO, it would be better to modify
from

bool MDSM::HaveEnoughDecodedVideo()

to

bool
MDSM::HaveEnoughDecodedVideo()
Attachment #8823932 - Flags: review?(kikuo) → review+

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8823932 [details]
Bug 1328781. part 2 - don't convert a double to an int64_t to avoid rounding errors.

https://reviewboard.mozilla.org/r/102394/#review102792

> It turns out that the style of this call and and the following |MDSM::HaveEnoughDecodedVideo| are the minority around here.
> 
> IMHO, it would be better to modify
> from
> 
> bool MDSM::HaveEnoughDecodedVideo()
> 
> to
> 
> bool
> MDSM::HaveEnoughDecodedVideo()

should add, the "original" style
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8823932 [details]
Bug 1328781. part 2 - don't convert a double to an int64_t to avoid rounding errors.

https://reviewboard.mozilla.org/r/102394/#review102792

> should add, the "original" style

Yes, that will happen in the next bug.
(Assignee)

Comment 7

2 years ago
Thanks for the review!

Comment 8

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b56ca2a1662
part 1 - somewhat rewrite the expression. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/1ba81f3c1fac
part 2 - don't convert a double to an int64_t to avoid rounding errors. r=kikuo

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b56ca2a1662
https://hg.mozilla.org/mozilla-central/rev/1ba81f3c1fac
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.