Remove MediaDecoder::mPausedForPlaybackRateNull

RESOLVED FIXED in Firefox 52

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Checking |mPlaybackRate == 0| is sufficient.
(Assignee)

Updated

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

Updated

2 years ago
Attachment #8794684 - Flags: review?(kikuo)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8794684 [details]
Bug 1304651 - Remove MediaDecoder::mPausedForPlaybackRateNull.

https://reviewboard.mozilla.org/r/80996/#review80524

::: dom/media/MediaDecoder.cpp:1513
(Diff revision 1)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +
> +  double oldRate = mPlaybackRate;
>    mPlaybackRate = aPlaybackRate;
> -  if (mPlaybackRate == 0.0) {
> +  if (aPlaybackRate == 0) {

nit:
aPlaybackRate is just assigned to mPlaybackRate. For my personal p.o.v., I'd prefer checking |if (mPlaybackRate == 0)| because current playback rate is already updated at first, and it's more straightforward to me that we should perform further actions according the current rate.  
But you may want to emphasize that the pause() will be performed if anyone's calling SetPlaybackRate with a rate 0.

I think both are fine.
Attachment #8794684 - Flags: review?(kikuo) → review+
(Assignee)

Comment 3

2 years ago
Thanks!

Comment 4

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac1c4519a19a
Remove MediaDecoder::mPausedForPlaybackRateNull. r=kikuo

Comment 5

2 years ago
mozreview-review
Comment on attachment 8794684 [details]
Bug 1304651 - Remove MediaDecoder::mPausedForPlaybackRateNull.

https://reviewboard.mozilla.org/r/80996/#review80578

::: dom/media/MediaDecoder.cpp:1513
(Diff revision 1)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +
> +  double oldRate = mPlaybackRate;
>    mPlaybackRate = aPlaybackRate;
> -  if (mPlaybackRate == 0.0) {
> +  if (aPlaybackRate == 0) {

I think it is better to use double == 0.0 than double == integer(0).

I'm not sure the compiler treats the comparison into double == static_cast<double>(0) or static_cast<int>(double) == 0 which may cause logic error.  

Do you intend to do it?
(Assignee)

Comment 6

2 years ago
(In reply to James Cheng[:JamesCheng] from comment #5)
> Comment on attachment 8794684 [details]
> Bug 1304651 - Remove MediaDecoder::mPausedForPlaybackRateNull.
> 
> https://reviewboard.mozilla.org/r/80996/#review80578
> 
> ::: dom/media/MediaDecoder.cpp:1513
> (Diff revision 1)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  double oldRate = mPlaybackRate;
> >    mPlaybackRate = aPlaybackRate;
> > -  if (mPlaybackRate == 0.0) {
> > +  if (aPlaybackRate == 0) {
> 
> I think it is better to use double == 0.0 than double == integer(0).
> 
> I'm not sure the compiler treats the comparison into double ==
> static_cast<double>(0) or static_cast<int>(double) == 0 which may cause
> logic error.  
> 
> Do you intend to do it?

I believe the 0 will be promoted to a double. 0.0 is just more verbose and doesn't give any benefit in this case.

Hi Gerald,
Can you share your language experience here?
Flags: needinfo?(gsquelart)
0.0 is a special value represents zero in IEEE 754. 

Maybe we should use static_cast<double>(0) to make it more readable and less confusion.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #6)
> I believe the 0 will be promoted to a double. 0.0 is just more verbose and
> doesn't give any benefit in this case.

Not that much experience, but I happened to look into this recently, and indeed numeric types around a binary operator are converted to a 'common' type, usually the one that covers a greater range:
http://en.cppreference.com/w/cpp/language/operator_arithmetic

In this case any integer type will be promoted to the floating type.
So 'aPlaybackRate == 0' is equivalent to 'aPlaybackRate == double(0)'.

I personally don't have a preference here.
I understand it could be a bit scary when not knowing the conversion rules, but I think these rules should probably be known -- they're quite intuitive (apart from differently-signed types of the same rank), especially compared to e.g. operator precedences.
So you two fight it out!
Flags: needinfo?(gsquelart)

Comment 9

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