VP9 Estimizer: isTypeSupported should test whether machine can decode 720p VP9 in real-time

RESOLVED FIXED in Firefox 47

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: ajones)

Tracking

(Depends on 2 bugs)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 wontfix, firefox47 fixed, firefox48 fixed, relnote-firefox 47+)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

4 years ago
We can use Firefox's idle service to schedule a background task to decode a 720p VP9 video clip. Anthony says this work should block enabling ffvp9 because we don't want to enable VP9 on low-end machines that can't decode in real-time.
Reporter

Comment 1

4 years ago
Until we he have time to implement the Right Thing solution using the idle service, the "Can this machine play 720p VP9 in real-time" function could just check a whitelist of hardware known to be fast enough. Enabling VP9 on a subset of compatible fast machines is better than not enabling it on any machines.
Depends on: 1227017
Priority: -- → P2
Flags: needinfo?(cpearce)
Reporter

Updated

3 years ago
Summary: isTypeSupported should test whether machine can decode 720p VP9 in real-time → VP9 Estimizer: isTypeSupported should test whether machine can decode 720p VP9 in real-time
Reporter

Updated

3 years ago
Blocks: 1198715
Reporter

Comment 2

3 years ago
cpearce, Anthony said you were going to investigate whether we should implement the VP9 performance estimizer or just use a whitelist of known good-enough hardware.
Assignee: nobody → cpearce
Priority: P2 → P1
Reporter

Comment 3

3 years ago
We should collect telemetry on the estimizer results and how many users are eligible for VP9.
Comment on attachment 8719354 [details]
MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance and enable VP9 on fast machines; r?jya

https://reviewboard.mozilla.org/r/34943/#review31567
Attachment #8719354 - Flags: review?(jyavenard) → review+
Comment on attachment 8719355 [details]
MozReview Request: Bug 1230265 - Unified build fix; r?jya

https://reviewboard.mozilla.org/r/34945/#review31569

Please split the patch between the implementation of Benchmark and its actual use in MediaSource

::: dom/media/Benchmark.h:11
(Diff revision 1)
> +#include "mozilla/Atomics.h"

You need to include the header for TimeStamp

this will not build on non-unified builds.

You're also including Atomics.h yet not using Atomics

::: dom/media/Benchmark.h:33
(Diff revision 1)
> +  virtual void Output(MediaData* aData) override;

the use of virtual keyword is redundant as override implies it's virtual.

::: dom/media/Benchmark.h:39
(Diff revision 1)
> +  RefPtr<Benchmark> mSelf;

This deserves some comment/explanation on its own.

::: dom/media/Benchmark.cpp:22
(Diff revision 1)
> +{

Need an assert that it's to be run on the main thread, as Preferences::GetUint will fail otherwise.

::: dom/media/Benchmark.cpp:58
(Diff revision 1)
> +{

Should have an assert for where things are supposed to run.
We started doing so everywhere in the media code:

so assert if not running on the main thread where it should be and OnTaskQueue()-like otherwise

::: dom/media/Benchmark.cpp:62
(Diff revision 1)
> +  demuxer->Init();

while this will work with the current WebM demuxer, this is not how it's supposed to be used.

Init() returns a promise, you can't assume that the promise will always be resolved successfully.

::: dom/media/Benchmark.cpp:70
(Diff revision 1)
> +                [this, trackDemuxer](

this holding on mSelf and this is a bit ugly. certainly not obvious at first glance.
Smart workaround though...

::: dom/media/Benchmark.cpp:78
(Diff revision 1)
> +                  mDecoder->Init();

MediaDataDecoder::Init() returns a promise, that too must be acted upon and not assuming it will have completed its run by the time Init() returns.

This will be especially more true of one day the Windows VP9 decoder is enabled.

::: dom/media/Benchmark.cpp:94
(Diff revision 1)
> +  mDecoder->Shutdown();

IRC, some decoders (in particular B2G ones) will crash/asser if you shut it down before it has been drained.

Keep in mind that ffvp9 won't be used on all platforms if there are alternative (Windows's HW vp9 decoder, or Android or OpenMax one)

::: dom/media/Benchmark.cpp:107
(Diff revision 1)
> +  if (mFrameCount == 1) {

I find it weird that you would start the timer after you've already decoded the first batch of samples.

It kinda skew the results as you're going to decode the same ones over and over and it's likely going to have code cached somewhere, not representing actual runs.

Can't you start the timer once you've finished demuxing all the samples instead?

::: dom/media/Benchmark.cpp:110
(Diff revision 1)
> +  if (mFrameCount == 300) {

300 should be a constant so it can be easily played with

::: dom/media/Benchmark.cpp:129
(Diff revision 1)
> +  mDecoder->Input(mSamples[mSampleIndex]);

mSamples is accessed over two different threads: when initialising/demuxing (main thread), and when InputExhausted is called (decoder's task queue).

While I see it as minor, as in theory those operations are not concurrently run, it's still a C++ undefined behaviour.

so mSamples should be protected by a monitor.

::: dom/media/mediasource/MediaSource.cpp:82
(Diff revision 1)
> -  return !mp4supported || !hwsupported;
> +  return !mp4supported || !hwsupported || Benchmark::IsVP9DecodeFast();

A pity this won't be known until we play the next YouTube video as the Benchmark wouldn't have completed its run yet.

Can't we call this earlier ?

::: modules/libpref/init/all.js:68
(Diff revision 1)
> -//pref("browser.cache.memory.capacity",     -1);
> +// pref("browser.cache.memory.capacity", -1);

all those changes to all.js are out of scope...

they should be done in another patch

I also can't tell what you've actually changed there
Attachment #8719355 - Flags: review?(jyavenard)
https://reviewboard.mozilla.org/r/34945/#review31603

Looks like we picked up some spurious changes in all.js ^^^
Comment on attachment 8719354 [details]
MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance and enable VP9 on fast machines; r?jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34943/diff/1-2/
Attachment #8719354 - Attachment description: MozReview Request: Bug 1230265 - Unified build fix; r?jya → MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance; r?jya
Comment on attachment 8719355 [details]
MozReview Request: Bug 1230265 - Unified build fix; r?jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34945/diff/1-2/
Attachment #8719355 - Attachment description: MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance and enable VP9 on fast machines; r?jya → MozReview Request: Bug 1230265 - Enable VP9 on fast machines; r?jya
Attachment #8719355 - Flags: review?(jyavenard)
Comment on attachment 8721116 [details]
MozReview Request: Bug 1230265 - Add VP9 benchmark telemetry; r?bsmedberg

https://reviewboard.mozilla.org/r/35579/#review32241

::: toolkit/components/telemetry/Histograms.json:8637
(Diff revision 1)
> +    "high": 1000,

1000fps!? That sounds excessive, don't you expect most users to be somewhere in the 5-120 range?

Since this is opt-out, it's got a higher data collection bar. Can you describe:

* the user value you are providing by collecting this data. I suspect you're going to correlate this against hardware profiles and whatnot, but it's not documented in the bug that I can see.
* Who will this metric be recorded for? Everyone just once, or a sample of people? Or can this be recorded multiple times per user? This should probably be described in the histogram "description". If it's just once, why can't this expire in 48 instead of 50?
* Have you tested this metric to make sure it records the things you care about? Sometimes we encourage people to start out with an opt-in metric, prove its value with beta, and then decide whether you still need the extra coverage from the entire population.
* Who is going to do the analysis? Is that bug filed and assigned?
Attachment #8721116 - Flags: review?(benjamin)
https://reviewboard.mozilla.org/r/34943/#review32259

::: dom/media/Benchmark.h:25
(Diff revision 2)
> +  void Init(UniquePtr<TrackInfo> aInfo,

TrackInfo&& aInfo

and use Move(mInfo) when called

::: dom/media/Benchmark.cpp:53
(Diff revision 2)
> +  RefPtr<Benchmark> ref = this;

use "self" , to keep consistency with the rest of the media code

::: dom/media/Benchmark.cpp:116
(Diff revision 2)
> +          mDecoderState.Init(track->GetInfo(), aHolder->mSamples);

personally I prefer mDecoderState.Init(Move(track->GetInfo), ...

but I guess it doesn't matter

::: dom/media/Benchmark.cpp:149
(Diff revision 2)
> +      Preferences::GetUint("media.benchmark.timeout", 1000)))

1s is a bit short as timeout no?

::: dom/media/Benchmark.cpp:192
(Diff revision 2)
> +  RefPtr<MediaData> data = aData;

this isn't necessary.
The callback do not need to take ownership of the MediaData

::: dom/media/Benchmark.cpp:205
(Diff revision 2)
> +    ref->Dispatch(NS_NewRunnableFunction([ref, decoder, decodeFps]() {

can't you just capture mDecoder ? it's already a RefPTr

::: dom/media/QueueObject.h:19
(Diff revision 2)
> +  QueueObject(RefPtr<AbstractThread> aThread);



::: dom/media/mediasource/MediaSource.cpp:82
(Diff revision 2)
> -  return !mp4supported || !hwsupported;
> +  return /*!mp4supported || !hwsupported ||*/ Benchmark::IsVP9DecodeFast();

You don't want that ,and you can remove patch #2 while at it
Comment on attachment 8719355 [details]
MozReview Request: Bug 1230265 - Unified build fix; r?jya

this patch shouldn't be there.
Attachment #8719355 - Flags: review?(jyavenard)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> Comment on attachment 8721116 [details]
> MozReview Request: Bug 1230265 - Add VP9 benchmark telemetry; r?bsmedberg
> 
> https://reviewboard.mozilla.org/r/35579/#review32241
> 
> ::: toolkit/components/telemetry/Histograms.json:8637
> (Diff revision 1)
> > +    "high": 1000,
> 
> 1000fps!? That sounds excessive, don't you expect most users to be somewhere
> in the 5-120 range?

No. My 4 year old PC is about 960fps at 720p. That equates to around 120fps at 4k.

> Since this is opt-out, it's got a higher data collection bar. Can you
> describe:
> 
> * the user value you are providing by collecting this data. I suspect you're
> going to correlate this against hardware profiles and whatnot, but it's not
> documented in the bug that I can see.

The value for the user is that we can make better decisions about whether VP9 will be an improvement over H.264 or not.

> * Who will this metric be recorded for? Everyone just once, or a sample of
> people? Or can this be recorded multiple times per user? This should
> probably be described in the histogram "description". If it's just once, why
> can't this expire in 48 instead of 50?

At this point it is just once. I would like to change it to once per release but I don't need to implement that in the first release.

> * Have you tested this metric to make sure it records the things you care
> about? Sometimes we encourage people to start out with an opt-in metric,
> prove its value with beta, and then decide whether you still need the extra
> coverage from the entire population.

I have not tested it.

I don't consider it an invasive metric in that you could independently derive the VP9 performance of someone's CPU by purchasing an identical CPU. You could use that to argue that opt-in provides the same information when combined with a list of CPU popularities. That would just be extra work.

> * Who is going to do the analysis? Is that bug filed and assigned?

Me and no.
Attachment #8719355 - Attachment is obsolete: true
Assignee: cpearce → ajones
Flags: needinfo?(cpearce)
Comment on attachment 8719355 [details]
MozReview Request: Bug 1230265 - Unified build fix; r?jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34945/diff/2-3/
Attachment #8719355 - Attachment description: MozReview Request: Bug 1230265 - Enable VP9 on fast machines; r?jya → MozReview Request: Bug 1230265 - Unified build fix; r?jya
Attachment #8719355 - Attachment is obsolete: false
Attachment #8719355 - Flags: review?(jyavenard)
Comment on attachment 8719354 [details]
MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance and enable VP9 on fast machines; r?jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34943/diff/2-3/
Attachment #8719354 - Attachment description: MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance; r?jya → MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance and enable VP9 on fast machines; r?jya
Attachment #8721116 - Flags: review?(benjamin)
Comment on attachment 8721116 [details]
MozReview Request: Bug 1230265 - Add VP9 benchmark telemetry; r?bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35579/diff/1-2/
Attachment #8719355 - Attachment is obsolete: true
Attachment #8719355 - Flags: review?(jyavenard)
Attachment #8719354 - Attachment is obsolete: true
Attachment #8721116 - Attachment is obsolete: true
Attachment #8721116 - Flags: review?(benjamin)
Attachment #8726113 - Flags: review?(jyavenard) → review+
Comment on attachment 8726113 [details]
MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance and enable VP9 on fast machines; r=jya

https://reviewboard.mozilla.org/r/37813/#review34351
Comment on attachment 8726114 [details]
MozReview Request: Bug 1230265 - Add VP9 benchmark telemetry; r?bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37815/diff/1-2/
Comment on attachment 8726113 [details]
MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance and enable VP9 on fast machines; r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37813/diff/1-2/
Comment on attachment 8726114 [details]
MozReview Request: Bug 1230265 - Add VP9 benchmark telemetry; r?bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37815/diff/2-3/
Comment on attachment 8726113 [details]
MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance and enable VP9 on fast machines; r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37813/diff/2-3/
Comment on attachment 8726114 [details]
MozReview Request: Bug 1230265 - Add VP9 benchmark telemetry; r?bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37815/diff/3-4/
The Benchmark class is now taking a MediaDataDemuxer argument. Options allow to decode any videos and measure the decoding speed.

Review commit: https://reviewboard.mozilla.org/r/38607/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38607/
Attachment #8727695 - Flags: review?(ajones)
Comment on attachment 8726114 [details]
MozReview Request: Bug 1230265 - Add VP9 benchmark telemetry; r?bsmedberg

https://reviewboard.mozilla.org/r/37815/#review35339

Please make sure that you have good testing of this metric and validate that it's collecting useful/meaningful data before it hits release. data-review=me
Attachment #8726114 - Flags: review?(benjamin) → review+
Comment on attachment 8727695 [details]
MozReview Request: Bug 1230265 - Add codec agnostic benchmark ; r?kentuckyfriedtakahe

https://reviewboard.mozilla.org/r/38607/#review35423

::: dom/media/Benchmark.cpp:63
(Diff revision 1)
> +          infile.read((char*)data->Elements(), fileSize);

This might be a good opportunity to create a talos test for decode performance.

::: dom/media/Benchmark.cpp:75
(Diff revision 1)
> +

Nice!

::: dom/media/Benchmark.cpp:291
(Diff revision 1)
> +        mSampleIndex == (size_t)ref->mParameters.mStopAtFrame) {

mStopAtFrame should be an optional type instead of using 0 as a sentinel.
Attachment #8727695 - Flags: review?(ajones) → review+
Depends on: 1256626
Comment on attachment 8726113 [details]
MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance and enable VP9 on fast machines; r=jya

Patch request is for all bugs

Approval Request Comment
[Feature/regressing bug #]: 1230265
[User impact if declined]: We aim to enable VP9 to all machines fast enough to support it. It would also reduce issues seen with h264 hardware decoder.
[Describe test coverage new/current, TreeHerder]: in central for a few weeks.
[Risks and why]: Low, people may complain of a cpu usage spike as youtube prefers Vp9 over h264 if available. Overall playback quality should be greater however
[String/UUID change made/needed]: none
Attachment #8726113 - Flags: approval-mozilla-aurora?
Reporter

Updated

3 years ago
Depends on: 1260305
Comment on attachment 8726113 [details]
MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance and enable VP9 on fast machines; r=jya

Media team wants to enable VP9 on faster machines in 47, I have been told this is mostly a staged rollout and quality is acceptable. Aurora47+
Attachment #8726113 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8726114 - Flags: approval-mozilla-aurora+
Attachment #8727695 - Flags: approval-mozilla-aurora+
Depends on: 1261129

Comment 42

3 years ago
The built-in benchmark video actually uses the VP8 codec, not VP9. This is OK if you assume performance is going to be roughly similar, which it usually is, but I suspect that on very old processors they might drift further apart.
Reporter

Updated

3 years ago
Depends on: 1265512
Reporter

Updated

3 years ago
No longer depends on: 1265512
Reporter

Comment 43

3 years ago
Release Note Request (optional, but appreciated)
[Why is this notable]: VP9 is an open, royalty-free codec that provides better video quality at a lower bandwidth. We're increasing the number of users for whom we're enabling VP9 from about 10% (people who don't have H.264) to 50% (people with fast enough machines).

[Suggested wording]: Enable VP9 video codec for users with fast machines.

[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Added to Fx47 (Beta) release notes.
Reporter

Updated

3 years ago
Depends on: 1285027
You need to log in before you can comment on or make changes to this bug.