Chris Peterson [:cpeterson] Reporter
	Description • 9 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.

	Chris Peterson [:cpeterson] Reporter
	Comment 1 • 9 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.

	Chris Peterson [:cpeterson] Reporter
	Comment 2 • 9 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.

	Chris Peterson [:cpeterson] Reporter
	Comment 3 • 9 years ago

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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 4 • 9 years ago

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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 5 • 9 years ago

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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 6 • 9 years ago

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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 7 • 9 years ago

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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 8 • 9 years ago

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

	Jean-Yves Avenard [:jya]
	Comment 9 • 9 years ago

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

	Jean-Yves Avenard [:jya]
	Comment 10 • 9 years ago

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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 11 • 9 years ago

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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 12 • 9 years ago

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

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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 13 • 9 years ago

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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 14 • 9 years ago

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/

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 15 • 9 years ago

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/

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 16 • 9 years ago

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

	Benjamin Smedberg
	Comment 17 • 9 years ago

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?

	Jean-Yves Avenard [:jya]
	Comment 18 • 9 years ago

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

	Jean-Yves Avenard [:jya]
	Comment 19 • 9 years ago

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

	Jean-Yves Avenard [:jya]
	Comment 20 • 9 years ago

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

this patch shouldn't be there.

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 21 • 9 years ago

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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 22 • 9 years ago

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/

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 23 • 9 years ago

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/

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 24 • 9 years ago

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/

	Jean-Yves Avenard [:jya]
	Comment 25 • 9 years ago

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

	Jean-Yves Avenard [:jya]
	Comment 26 • 9 years ago

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

	Jean-Yves Avenard [:jya]
	Comment 27 • 9 years ago

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

	Jean-Yves Avenard [:jya]
	Comment 28 • 9 years ago

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/

	Jean-Yves Avenard [:jya]
	Comment 29 • 9 years ago

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/

	Jean-Yves Avenard [:jya]
	Comment 30 • 9 years ago

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/

	Jean-Yves Avenard [:jya]
	Comment 31 • 9 years ago

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/

	Jean-Yves Avenard [:jya]
	Comment 32 • 9 years ago

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/

	Jean-Yves Avenard [:jya]
	Comment 33 • 9 years ago

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/

	Benjamin Smedberg
	Comment 34 • 9 years ago

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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 35 • 9 years ago

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.

	Pulsebot
	Comment 36 • 9 years ago

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

	Pulsebot
	Comment 37 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 38 • 9 years ago
bugherder

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

	Jean-Yves Avenard [:jya]
	Comment 39 • 9 years ago

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

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 40 • 9 years ago

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+

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 41 • 9 years ago
bugherder uplift

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

	depaner
	Comment 42 • 9 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.

	Chris Peterson [:cpeterson] Reporter
	Comment 43 • 9 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)]:

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 44 • 9 years ago

Added to Fx47 (Beta) release notes.