Disable VP9 estimizer on Mac.

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Unspecified
macOS
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57+ fixed, firefox58 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
Spawn from bug 1400787 comment 6.
Assignee

Updated

2 years ago
Blocks: 1400787
Assignee

Updated

2 years ago
Assignee: nobody → kaku
Status: NEW → ASSIGNED
Assignee

Comment 1

2 years ago
(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.
Flags: needinfo?(bwu)
(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?
Flags: needinfo?(bwu) → needinfo?(kaku)
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..
Assignee

Comment 4

2 years ago
(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.
Flags: needinfo?(kaku) → needinfo?(bwu)
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.
(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
(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.
Flags: needinfo?(bwu)
Assignee

Comment 8

2 years ago
(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.
(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.
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 hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
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"
Attachment #8912777 - Flags: review?(jyavenard) → review+
Assignee

Comment 13

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

Comment 14

2 years ago
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.
Assignee

Comment 15

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review-reply
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
Assignee

Updated

2 years ago
Summary: Disable VP9 on Mac. → Disable VP9 estimizer on Mac.
Comment hidden (mozreview-request)

Comment 18

2 years ago
Pushed by tkuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a46ed72cd5f
disable VP9 estimizer on Mac; r=jya
https://hg.mozilla.org/mozilla-central/rev/6a46ed72cd5f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
[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.
Kaku,
Please request a uplift when you are available.
Thanks.
Flags: needinfo?(kaku)
Assignee

Comment 22

2 years ago
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.
Flags: needinfo?(kaku)
Attachment #8912777 - Flags: approval-mozilla-beta?
Comment on attachment 8912777 [details]
Bug 1403412 - disable VP9 estimizer on Mac;

ok, let's take it
Should be in 57b5
Attachment #8912777 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.