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

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

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.

Comment 1

[Chris Peterson [:cpeterson]]

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.

Comment 2

[Chris Peterson [:cpeterson]]

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.

Comment 3

[Chris Peterson [:cpeterson]]

We should collect telemetry on the estimizer results and how many users are eligible for VP9.

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ae1f6552e2e

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e10d031c07e5

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63e4bb4016f8

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance and enable VP9 on fast machines; r?jya

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

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: MozReview Request: Bug 1230265 - Unified build fix; r?jya

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

Comment 9

[Jean-Yves Avenard [:jya]]

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

Comment 10

[Jean-Yves Avenard [:jya]]

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

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Looks like we picked up some spurious changes in all.js ^^^

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc1b85a5a3a8

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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/

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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/

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: MozReview Request: Bug 1230265 - Add VP9 benchmark telemetry; r?bsmedberg

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

Comment 17

[Benjamin Smedberg]

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?

Comment 18

[Jean-Yves Avenard [:jya]]

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 19

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/34943/#review32435

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8719355 [details]
MozReview Request: Bug 1230265 - Unified build fix; r?jya

this patch shouldn't be there.

Comment 21

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(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.

Comment 22

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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/

Comment 23

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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/

Comment 24

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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/

Comment 25

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance and enable VP9 on fast machines; r=jya

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

Comment 26

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1230265 - Add VP9 benchmark telemetry; r?bsmedberg

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

Comment 27

[Jean-Yves Avenard [:jya]]

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 28

[Jean-Yves Avenard [:jya]]

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 29

[Jean-Yves Avenard [:jya]]

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 30

[Jean-Yves Avenard [:jya]]

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 31

[Jean-Yves Avenard [:jya]]

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 32

[Jean-Yves Avenard [:jya]]

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/

Comment 33

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1230265 - Add codec agnostic benchmark ; r?kentuckyfriedtakahe

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/

Comment 34

[Benjamin Smedberg]

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

Comment 35

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/604c7c7a01f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/31edb28d9b37
https://hg.mozilla.org/integration/mozilla-inbound/rev/360956c65923

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5082714d41

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/604c7c7a01f3
https://hg.mozilla.org/mozilla-central/rev/31edb28d9b37
https://hg.mozilla.org/mozilla-central/rev/360956c65923
https://hg.mozilla.org/mozilla-central/rev/5f5082714d41

Comment 39

[Jean-Yves Avenard [:jya]]

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

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4e5aa8d948fc
https://hg.mozilla.org/releases/mozilla-aurora/rev/8ea687a1945c
https://hg.mozilla.org/releases/mozilla-aurora/rev/921e6142e8b9
https://hg.mozilla.org/releases/mozilla-aurora/rev/dec341c282e8

Comment 42

[depaner]

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.

Comment 43

[Chris Peterson [:cpeterson]]

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)]:

Comment 44

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx47 (Beta) release notes.