Closed
Bug 1230265
Opened 9 years ago
Closed 9 years ago
VP9 Estimizer: isTypeSupported should test whether machine can decode 720p VP9 in real-time
Categories
(Core :: Audio/Video: Playback, defect, P1)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: cpeterson, Assigned: ajones)
References
(Depends on 2 open bugs)
Details
Attachments
(3 files, 3 obsolete files)
(deleted),
text/x-review-board-request
|
jya
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
(deleted),
text/x-review-board-request
|
benjamin
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
(deleted),
text/x-review-board-request
|
ajones
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
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•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.
Depends on: 1227017
Updated•9 years ago
|
Priority: -- → P2
Updated•9 years ago
|
Flags: needinfo?(cpearce)
Reporter | ||
Updated•9 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 | ||
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.
Assignee: nobody → cpearce
Priority: P2 → P1
Reporter | ||
Comment 3•9 years ago
|
||
We should collect telemetry on the estimizer results and how many users are eligible for VP9.
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ae1f6552e2e
Assignee | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e10d031c07e5
Assignee | ||
Comment 6•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63e4bb4016f8
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/
Attachment #8719354 -
Flags: review?(jyavenard)
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/
Attachment #8719355 -
Flags: review?(jyavenard)
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
Attachment #8719354 -
Flags: review?(jyavenard) → review+
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
Attachment #8719355 -
Flags: review?(jyavenard)
Assignee | ||
Comment 12•9 years ago
|
||
https://reviewboard.mozilla.org/r/34945/#review31603 Looks like we picked up some spurious changes in all.js ^^^
Assignee | ||
Comment 13•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc1b85a5a3a8
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/
Attachment #8719354 -
Attachment description: MozReview Request: Bug 1230265 - Unified build fix; r?jya → MozReview Request: Bug 1230265 - Benchmark VP9 decoder performance; r?jya
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/
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)
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/
Attachment #8721116 -
Flags: review?(benjamin)
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?
Attachment #8721116 -
Flags: review?(benjamin)
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
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.
Attachment #8719355 -
Flags: review?(jyavenard)
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8719355 -
Attachment is obsolete: true
Updated•9 years ago
|
Assignee: cpearce → ajones
Flags: needinfo?(cpearce)
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/
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)
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/
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
Assignee | ||
Updated•9 years ago
|
Attachment #8721116 -
Flags: review?(benjamin)
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/
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/
Attachment #8726113 -
Flags: review?(jyavenard)
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/
Attachment #8726114 -
Flags: review?(benjamin)
Updated•9 years ago
|
Attachment #8719355 -
Attachment is obsolete: true
Attachment #8719355 -
Flags: review?(jyavenard)
Updated•9 years ago
|
Attachment #8719354 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8721116 -
Attachment is obsolete: true
Attachment #8721116 -
Flags: review?(benjamin)
Updated•9 years ago
|
Attachment #8726113 -
Flags: review?(jyavenard) → review+
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
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/
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/
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/
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/
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/
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/
Attachment #8727695 -
Flags: review?(ajones)
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
Attachment #8726114 -
Flags: review?(benjamin) → review+
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.
Attachment #8727695 -
Flags: review?(ajones) → review+
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
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
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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
Attachment #8726113 -
Flags: approval-mozilla-aurora?
Reporter | ||
Updated•9 years ago
|
status-firefox46:
--- → wontfix
status-firefox47:
--- → ?
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+
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
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.
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)]:
relnote-firefox:
--- → ?
Added to Fx47 (Beta) release notes.
You need to log in
before you can comment on or make changes to this bug.
Description
•