1403412 - Disable VP9 estimizer on Mac.

Status: RESOLVED FIXED

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

Description

[Tzuhao Kuo [:kaku]]

Spawn from bug 1400787 comment 6.

Comment 1

[Tzuhao Kuo [:kaku]]

(In reply to Blake Wu [:bwu][:blakewu] from bug 1400787 comment #6)
> Per discussion with Anthony, it would be better to disable VP9 and re-enable
> it once those bugs in bug 1400787 comment 4 are fixed. 
> Kaku,
> Can you help disable VP9 decoder on Mac?

Hmm......, we only want to disable VP9 for Yourtube on Mac, right? 
Which means that we will keep the VP9 decoding capbility on Mac. 
Say, we have a local WebM/VP9 video file, we should be able to play it.

Comment 2

[Blake Wu [:bwu][:blakewu]]

(In reply to Tzuhao Kuo [:kaku] (PTO 9/27) from comment #1)
> (In reply to Blake Wu [:bwu][:blakewu] from bug 1400787 comment #6)
> > Per discussion with Anthony, it would be better to disable VP9 and re-enable
> > it once those bugs in bug 1400787 comment 4 are fixed. 
> > Kaku,
> > Can you help disable VP9 decoder on Mac?
> 
> Hmm......, we only want to disable VP9 for Yourtube on Mac, right? 
> Which means that we will keep the VP9 decoding capbility on Mac. 
> Say, we have a local WebM/VP9 video file, we should be able to play it.
I think we just disable VP9 for all cases. 
But why we should treat them different? Local file requires less rendering effort?

Comment 3

[Blake Wu [:bwu][:blakewu]]

Second thought. Can you check if disabling VP9 fixes the problem? I am not quite sure about it. H.264 data may use the same rendering pipeline..

Comment 4

[Tzuhao Kuo [:kaku]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #2)
> (In reply to Tzuhao Kuo [:kaku] (PTO 9/27) from comment #1)
> > (In reply to Blake Wu [:bwu][:blakewu] from bug 1400787 comment #6)
> > > Per discussion with Anthony, it would be better to disable VP9 and re-enable
> > > it once those bugs in bug 1400787 comment 4 are fixed. 
> > > Kaku,
> > > Can you help disable VP9 decoder on Mac?
> > 
> > Hmm......, we only want to disable VP9 for Yourtube on Mac, right? 
> > Which means that we will keep the VP9 decoding capbility on Mac. 
> > Say, we have a local WebM/VP9 video file, we should be able to play it.
> I think we just disable VP9 for all cases. 
> But why we should treat them different? Local file requires less rendering
> effort?
No, it's because that Youtube will send us stream in other formats. 
But if we disable VP9 as a whole, you will never be able to play WebM/VP9 videos.
Is your intesion to "not support VP9 codec"?

(In reply to Blake Wu [:bwu][:blakewu] from comment #3)
> Second thought. Can you check if disabling VP9 fixes the problem? I am not
> quite sure about it. H.264 data may use the same rendering pipeline..

See bug 1400787 comment 1. Switching to H.264 does ameliorate.

Comment 5

[Jean-Yves Avenard [:jya]]

The pessimist in me thinks that now we'll never see the issue in the compositor fixed.


The safest way is to either disable the VP9 estimizer or to bump the threshold value to a much greater value. Like 250.

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #3)
> Second thought. Can you check if disabling VP9 fixes the problem? I am not
> quite sure about it. H.264 data may use the same rendering pipeline..

H264 uses the Apple VideoToolbox decoder.
It outputs IOSurface they can be Gpu backed.

Disabling VP9 only with MSE is typically enough for YouTube. They are by far the main user of this codec,

The other alternative is to only enable VP9 on machines with a hardware decoder, that will exclude Mac.

I agree with kaku, A blanket rule to refuse VP9 would be a big step backwards IMHO

Comment 7

[Blake Wu [:bwu][:blakewu]]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> The pessimist in me thinks that now we'll never see the issue in the
> compositor fixed.
We could file another bug to re-enable VP9 and track it on 58. 
> 
> 
> The safest way is to either disable the VP9 estimizer or to bump the
> threshold value to a much greater value. Like 250.
Good idea.

Comment 8

[Tzuhao Kuo [:kaku]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #7)
> (In reply to Jean-Yves Avenard [:jya] from comment #5)
> > The pessimist in me thinks that now we'll never see the issue in the
> > compositor fixed.
> We could file another bug to re-enable VP9 and track it on 58. 
> > 
> > 
> > The safest way is to either disable the VP9 estimizer or to bump the
> > threshold value to a much greater value. Like 250.
> Good idea.

I would like to go with disabling the VP9 estimizer on MAC because I have no idea on what "a greater threshold" should be.

A patch is following.

Comment 9

[Jean-Yves Avenard [:jya]]

(In reply to Tzuhao Kuo [:kaku] (PTO 9/27) from comment #8)
> I would like to go with disabling the VP9 estimizer on MAC because I have no
> idea on what "a greater threshold" should be.
> 
> A patch is following.

I could run the measurement on a quad-core MBP 15" late 2016. They are very fast machine and we can use it as base model. So only later model would be supported implicitly.

Comment 10

[Jean-Yves Avenard [:jya]]

My 3GHz 8-core mac pro late 2013 gives me 227
The 2.8GHz quad-core i7 (6th gen) gives 180.

It's currently set at 150.

Comment 11

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1403412 - disable VP9 estimizer on Mac;

Review commit: https://reviewboard.mozilla.org/r/184092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184092/

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8912777 [details]
Bug 1403412 - disable VP9 estimizer on Mac;

https://reviewboard.mozilla.org/r/184092/#review189246

::: commit-message-70158:1
(Diff revision 1)
> +Bug 1403412 - disable VP9 with MSE on Mac; r?jya

the commit title doesn't match the change.

You're disabling the "estimizer"

Comment 13

[Tzuhao Kuo [:kaku]]

@jya, sorry that I didn't noticed your comment. It seems that you would like to have a discussion about the threshold?

Comment 14

[Tzuhao Kuo [:kaku]]

My MacBook Pro (Retina, 15-inch, Mid 2014) with 2.2 GHz Intel Core i7 gives 198.
But, the estimated number highly depends on the system's runtime loading.

Comment 15

[Tzuhao Kuo [:kaku]]

Comment on attachment 8912777 [details]
Bug 1403412 - disable VP9 estimizer on Mac;

https://reviewboard.mozilla.org/r/184092/#review189246

> the commit title doesn't match the change.
> 
> You're disabling the "estimizer"

Do you prefer to have an explicit logic in the `IsWebMForced()` method to disable WebM/VP9 on Mac?
(http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/dom/media/mediasource/MediaSource.cpp#76)

Say:

```
static bool
IsWebMForced(DecoderDoctorDiagnostics* aDiagnostics)
{
#ifdef MOZ_APPLEMEDIA
  return false;
#endif 
}
```

or, just go with the current patch?

BTW, is VPX the only codec that WebM might have?

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8912777 [details]
Bug 1403412 - disable VP9 estimizer on Mac;

https://reviewboard.mozilla.org/r/184092/#review189246

> Do you prefer to have an explicit logic in the `IsWebMForced()` method to disable WebM/VP9 on Mac?
> (http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/dom/media/mediasource/MediaSource.cpp#76)
> 
> Say:
> 
> ```
> static bool
> IsWebMForced(DecoderDoctorDiagnostics* aDiagnostics)
> {
> #ifdef MOZ_APPLEMEDIA
>   return false;
> #endif 
> }
> ```
> 
> or, just go with the current patch?
> 
> BTW, is VPX the only codec that WebM might have?

no, i prefer your current implementation. the other would prevent some machines that would otherwise get it (like my mac pro thst doesnt have hardware decoding enabled)

just the commit description needs a small tweak

Comment 17

[Tzuhao Kuo [:kaku]]

Comment on attachment 8912777 [details]
Bug 1403412 - disable VP9 estimizer on Mac;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184092/diff/1-2/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a46ed72cd5f
disable VP9 estimizer on Mac; r=jya

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6a46ed72cd5f

Comment 20

[Blake Wu [:bwu][:blakewu]]

[Tracking Requested - why for this release]:
Move the track on bug 1400787 here. Quote from bug 1400787 comment 3 as below. 
This is unnecessary and excessive power usage. We need to find a fix for this in 57. We can ameliorate the problem by disabling VP9 on YouTube but that won't fix Netflix.

Comment 21

[Blake Wu [:bwu][:blakewu]]

Kaku,
Please request a uplift when you are available.
Thanks.

Comment 22

[Tzuhao Kuo [:kaku]]

Comment on attachment 8912777 [details]
Bug 1403412 - disable VP9 estimizer on Mac;

Approval Request Comment
[Feature/Bug causing the regression]: the main reason why we have this fix is in bug 1400787.
[User impact if declined]: watching VP9 video (the default of Youtube) ramps up computer fans on Macbook.
[Is this code covered by automated tests?]: yes.
[Has the fix been verified in Nightly?]: yes.
[Needs manual test from QE? If yes, steps to reproduce]: no.
[List of other uplifts needed for the feature/fix]: no.
[Is the change risky?]: no.
[Why is the change risky/not risky?]: this patch makes the IsVP9DecodeFast() helper function always to return false on Mac. In this way, Firefox will not force using WebM on Mac for MSE, and Firefox will ask the streaming server to send other formats. This is not risky because that IsVP9DecodeFast() works as an estimizer to see if the system is good for decoding VP9 or not, If the system is not good enough, IsVP9DecodeFast() returns false and triggers the fallback mechanism, which has been in Firefox for a long time and worked well.
[String changes made/needed]: no.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8912777 [details]
Bug 1403412 - disable VP9 estimizer on Mac;

ok, let's take it
Should be in 57b5

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/452ca306a258

Comment 25

[Andrei Vaida [:avaida]]

(In reply to Tzuhao Kuo [:kaku] from comment #22)
> [Is this code covered by automated tests?]: yes.
> [Has the fix been verified in Nightly?]: yes.
> [Needs manual test from QE? If yes, steps to reproduce]: no.

Setting qe-verify- based on Tzuhao Kuo's assessment on manual testing needs and the fact that this fix has automated coverage.