Disable VP9 estimizer on Mac.

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

(Blocks: 1 bug)

unspecified
mozilla58
Unspecified
macOS
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57+ fixed, firefox58 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Spawn from bug 1400787 comment 6.
(Assignee)

Updated

a year ago
Blocks: 1400787
(Assignee)

Updated

a year ago
Assignee: nobody → kaku
Status: NEW → ASSIGNED
(Assignee)

Comment 1

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

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

Comment 18

a year 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
Last Resolved: a year ago
status-firefox58: --- → fixed
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.
status-firefox57: --- → affected
tracking-firefox57: --- → ?
Kaku,
Please request a uplift when you are available.
Thanks.
Flags: needinfo?(kaku)
tracking-firefox57: ? → +
(Assignee)

Comment 22

a year 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+

Comment 24

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/452ca306a258
status-firefox57: affected → fixed
(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.