Closed Bug 1263839 Opened 9 years ago Closed 9 years ago

VP9 benchmark is using VP8 sample; needs to be VP9

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: ajones, Assigned: jya)

References

Details

Attachments

(2 files, 1 obsolete file)

MozReview Request: Bug 1263839 - WebM is now actually a VP9 sample; r?jya
58 bytes, text/x-review-board-request
jya
: review+
ritu
: approval-mozilla-aurora+
Details
MozReview Request: Bug 1263839: P2. Force re-run of VP9 benchmark based on a version check. r?kentuckyfriedtakahe
58 bytes, text/x-review-board-request
ajones
: review+
ritu
: approval-mozilla-aurora+
Details
The VP9 benchmark is using a VP8 sample using the wrong codec parameters.
Priority: -- → P1
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/
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+
https://hg.mozilla.org/mozilla-central/rev/1cbabad59a63 https://hg.mozilla.org/mozilla-central/rev/bca6a0b5816f
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Attachment #8740294 - Attachment is obsolete: true
Attachment #8740294 - Flags: review?(jyavenard)
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?
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+
These patches seem to have caused a ~240KB size increase in the Firefox for Android APK. Was that intended?
(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)
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?
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: 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)
It is a single word change. But prevent this uplift to apply cleanly.
Flags: needinfo?(jyavenard)
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)
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.
Depends on: 1268456
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: