Closed Bug 1304651 Opened 6 years ago Closed 6 years ago

Remove MediaDecoder::mPausedForPlaybackRateNull

Categories

(Core :: Audio/Video: Playback, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(1 file)

Checking |mPlaybackRate == 0| is sufficient.
Assignee: nobody → jwwang
Priority: -- → P3
Attachment #8794684 - Flags: review?(kikuo)
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+
Thanks!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac1c4519a19a
Remove MediaDecoder::mPausedForPlaybackRateNull. r=kikuo
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?
(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)
https://hg.mozilla.org/mozilla-central/rev/ac1c4519a19a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.