VP9 benchmark is using VP8 sample; needs to be VP9

RESOLVED FIXED in Firefox 47

Status

()

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kentuckyfriedtakahe, Assigned: jya)

Tracking

(Depends on: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

The VP9 benchmark is using a VP8 sample using the wrong codec parameters.
Created attachment 8740294 [details]
MozReview Request: Bug 1263839 - WebM is now actually a VP9 sample; r?jya

Review commit: https://reviewboard.mozilla.org/r/45711/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45711/
Attachment #8740294 - Flags: review?(jyavenard)
Priority: -- → P1
(Assignee)

Comment 2

3 years ago
Created attachment 8740758 [details]
MozReview Request: Bug 1263839 - WebM is now actually a VP9 sample; r?jya

Review commit: https://reviewboard.mozilla.org/r/46007/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46007/
Attachment #8740758 - Flags: review?(jyavenard)
Attachment #8740759 - Flags: review?(ajones)
(Assignee)

Comment 3

3 years ago
Created attachment 8740759 [details]
MozReview Request: Bug 1263839: P2. Force re-run of VP9 benchmark based on a version check. r?kentuckyfriedtakahe

The version number is to be manually updated when we want to re-run the test (like improvement in ffvp9 or libvpx)

Review commit: https://reviewboard.mozilla.org/r/46009/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46009/
(Assignee)

Comment 4

3 years ago
Comment on attachment 8740758 [details]
MozReview Request: Bug 1263839 - WebM is now actually a VP9 sample; r?jya

https://reviewboard.mozilla.org/r/46007/#review42513

I did check this time that it was VP9 :)

decoding speed is 1/3rd of the earlier VP8 one!
Attachment #8740758 - Flags: review?(jyavenard) → review+
Comment on attachment 8740759 [details]
MozReview Request: Bug 1263839: P2. Force re-run of VP9 benchmark based on a version check. r?kentuckyfriedtakahe

https://reviewboard.mozilla.org/r/46009/#review42809
Attachment #8740759 - Flags: review?(ajones) → review+

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1cbabad59a63
https://hg.mozilla.org/mozilla-central/rev/bca6a0b5816f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Updated

3 years ago
status-firefox47: --- → affected
(Assignee)

Updated

3 years ago
Attachment #8740294 - Attachment is obsolete: true
Attachment #8740294 - Flags: review?(jyavenard)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8740758 [details]
MozReview Request: Bug 1263839 - WebM is now actually a VP9 sample; r?jya

Approval Request Comment
[Feature/regressing bug #]: 1263839
[User impact if declined]: slow machines may incorrectly determine that they are fast enough to decode VP9 when they aren't. The original sample should have been VP9, however a typo in the command used to generate the sample caused it to actually use VP8
[Describe test coverage new/current, TreeHerder]: manual test, gtest
[Risks and why]: Very low, we actually reduce the risk of regression as the likelihood of enabling the new code is reduced.
[String/UUID change made/needed]: none
Attachment #8740758 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 9

3 years ago
uplift request is for both patches
Comment on attachment 8740758 [details]
MozReview Request: Bug 1263839 - WebM is now actually a VP9 sample; r?jya

Fixes the logic to determine which machines can handle VP9, Aurora47+
Attachment #8740758 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

3 years ago
Attachment #8740759 - Flags: approval-mozilla-aurora+
These patches seem to have caused a ~240KB size increase in the Firefox for Android APK. Was that intended?
(Assignee)

Comment 13

3 years ago
(In reply to Mark Finkle (:mfinkle) from comment #11)
> These patches seem to have caused a ~240KB size increase in the Firefox for
> Android APK. Was that intended?

This change added 8 1280x720 compressed VP9 frames. That takes space (not much considering that decompressed those would take 11059200 bytes)
seems this has conflicts uplifting 

grafting 339749:1cbabad59a63 "Bug 1263839 - WebM is now actually a VP9 sample; r=me"
merging dom/media/WebMSample.h
warning: conflicts while merging dom/media/WebMSample.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(jyavenard)
(Assignee)

Comment 15

3 years ago
bug 1261129 needs to be uplifted as well, it's one word change
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #13)
> (In reply to Mark Finkle (:mfinkle) from comment #11)
> > These patches seem to have caused a ~240KB size increase in the Firefox for
> > Android APK. Was that intended?
> 
> This change added 8 1280x720 compressed VP9 frames. That takes space (not
> much considering that decompressed those would take 11059200 bytes)

Thanks for the details, but could you provide more context? The bug summary mentions "benchmark" so I am wondering why we need to ship frames for a benchmark. What is the purpose of the frames?
(Assignee)

Comment 17

3 years ago
We benchmark VP9 decoding speed and whitelist MSE sites using VP9 (e.g. youtube). For machines deemed too slow we only offer H264

see bug 1230265
(Assignee)

Updated

3 years ago
Assignee: nobody → jyavenard
(In reply to Mark Finkle (:mfinkle) from comment #16)
> Thanks for the details, but could you provide more context? The bug summary
> mentions "benchmark" so I am wondering why we need to ship frames for a
> benchmark. What is the purpose of the frames?

On desktop we don't have a good way to decide whether to advertise VP9 support. We run a benchmark to see if the machine is "reasonably fast". We can run the same code on Android to help get the best overall codec to users. However we could arbitrarily decode on Android by some other means, such as the Android version or the availability of hardware VP9 decoding.

For now we could just exclude Android from the benchmark.
(In reply to Jean-Yves Avenard [:jya] from comment #15)
> bug 1261129 needs to be uplifted as well, it's one word change

liz, can we have approval for this ?
Flags: needinfo?(lhenry)
I'm not sure what you're asking for here, uplift in bug 1261129?  for 47 or 46 or both?  Best to request uplift in the relevant bug (looking there, I'm still not sure what we need to do)
Flags: needinfo?(lhenry) → needinfo?(jyavenard)
(Assignee)

Comment 21

3 years ago
It is a single word change. But prevent this uplift to apply cleanly.
Flags: needinfo?(jyavenard)
(Assignee)

Updated

3 years ago
Depends on: 1261129
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #18)
> (In reply to Mark Finkle (:mfinkle) from comment #16)
> > Thanks for the details, but could you provide more context? The bug summary
> > mentions "benchmark" so I am wondering why we need to ship frames for a
> > benchmark. What is the purpose of the frames?
> 
> On desktop we don't have a good way to decide whether to advertise VP9
> support. We run a benchmark to see if the machine is "reasonably fast". We
> can run the same code on Android to help get the best overall codec to
> users. However we could arbitrarily decode on Android by some other means,
> such as the Android version or the availability of hardware VP9 decoding.
> 
> For now we could just exclude Android from the benchmark.

If we aren't running the benchmark, can we stop including the sample frames? We have some folks actively trying to reduce the APK size, so stuff like this is frustrating.
Flags: needinfo?(ajones)
Filed bug 1266102 with a patch to disable the benchmark (and inclusion of the samples) on Android
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #22)
> If we aren't running the benchmark, can we stop including the sample frames?
> We have some folks actively trying to reduce the APK size, so stuff like
> this is frustrating.

There is value in running the benchmark on Android but it does create extra work.
Flags: needinfo?(ajones)
(Assignee)

Comment 25

3 years ago
we are running the benchmark on Android...
a small bump in the android installer size with this:
https://treeherder.mozilla.org/perf.html#/alerts?id=939
oh, not just android, but windows and linux as well.

Updated

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