Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed)

Details

Attachments

(9 attachments)

58 bytes, text/x-review-board-request
ajones
: review+
Details
58 bytes, text/x-review-board-request
ajones
: review+
Details
58 bytes, text/x-review-board-request
ajones
: review+
Details
58 bytes, text/x-review-board-request
ajones
: review+
Details
58 bytes, text/x-review-board-request
ajones
: review+
Details
58 bytes, text/x-review-board-request
ajones
: review+
Details
58 bytes, text/x-review-board-request
ajones
: review+
Details
58 bytes, text/x-review-board-request
ajones
: review+
Details
58 bytes, text/x-review-board-request
glandium
: review+
Details
Assignee

Description

3 years ago
This will serve as example for the fuzzing team to decode a local file.
Assignee

Updated

3 years ago
Blocks: 1236702
Assignee

Comment 2

3 years ago
We were dispatching a task on the main thread, only to redispatch it immediately.

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

Comment 6

3 years ago
Comment on attachment 8728357 [details]
MozReview Request: Bug 1254858: P1. Remove unused member. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38915/diff/1-2/
Assignee

Comment 7

3 years ago
Comment on attachment 8728358 [details]
MozReview Request: Bug 1254858: P2. Properly dispatch task on the right thread. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38917/diff/1-2/
Assignee

Comment 8

3 years ago
Comment on attachment 8728359 [details]
MozReview Request: Bug 1254858: P3. reorganise Benchmark classes declarations to only expose usable members. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38919/diff/1-2/
Assignee

Comment 9

3 years ago
Comment on attachment 8728360 [details]
MozReview Request: Bug 1254858: P4. Allow to pass mimetype in constructor. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38921/diff/1-2/
Assignee

Comment 10

3 years ago
Comment on attachment 8728361 [details]
MozReview Request: Bug 1254858: P5. Add h264 decoding gtest. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38923/diff/1-2/
Comment on attachment 8728357 [details]
MozReview Request: Bug 1254858: P1. Remove unused member. r=kentuckyfriedtakahe

https://reviewboard.mozilla.org/r/38915/#review35757
Attachment #8728357 - Flags: review?(ajones) → review+
Comment on attachment 8728358 [details]
MozReview Request: Bug 1254858: P2. Properly dispatch task on the right thread. r=kentuckyfriedtakahe

https://reviewboard.mozilla.org/r/38917/#review35759
Attachment #8728358 - Flags: review?(ajones) → review+
Assignee

Comment 13

3 years ago
Comment on attachment 8728361 [details]
MozReview Request: Bug 1254858: P5. Add h264 decoding gtest. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38923/diff/2-3/
Assignee

Comment 16

3 years ago
When running gtest, none of the prefs actually exist, so we end up having most PDMs disabled.

Review commit: https://reviewboard.mozilla.org/r/39113/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39113/
Attachment #8728778 - Flags: review?(ajones)
Comment on attachment 8728359 [details]
MozReview Request: Bug 1254858: P3. reorganise Benchmark classes declarations to only expose usable members. r=kentuckyfriedtakahe

https://reviewboard.mozilla.org/r/38919/#review35761
Attachment #8728359 - Flags: review?(ajones) → review+
Attachment #8728360 - Flags: review?(ajones) → review+
Comment on attachment 8728360 [details]
MozReview Request: Bug 1254858: P4. Allow to pass mimetype in constructor. r=kentuckyfriedtakahe

https://reviewboard.mozilla.org/r/38921/#review35763
Comment on attachment 8728361 [details]
MozReview Request: Bug 1254858: P5. Add h264 decoding gtest. r=kentuckyfriedtakahe

https://reviewboard.mozilla.org/r/38923/#review35765
Attachment #8728361 - Flags: review?(ajones) → review+
Attachment #8728776 - Flags: review?(ajones) → review+
Comment on attachment 8728776 [details]
MozReview Request: Bug 1254858: P6. Add VP9 decoding gtest. r=kentuckyfriedtakahe

https://reviewboard.mozilla.org/r/39109/#review35767
Comment on attachment 8728777 [details]
MozReview Request: Bug 1254858: P7. Add logging if libmozav can't be found. r=kentuckyfriedtakahe

https://reviewboard.mozilla.org/r/39111/#review35769
Attachment #8728777 - Flags: review?(ajones) → review+
Comment on attachment 8728778 [details]
MozReview Request: Bug 1254858: P8. Change default preferences value if prefs don't exist. r=kentuckyfriedtakahe

https://reviewboard.mozilla.org/r/39113/#review35771
Attachment #8728778 - Flags: review?(ajones) → review+
Assignee

Comment 23

3 years ago
Comment on attachment 8728777 [details]
MozReview Request: Bug 1254858: P7. Add logging if libmozav can't be found. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39111/diff/1-2/
Attachment #8728777 - Attachment description: MozReview Request: Bug 1254858: P7. Add logging if libmozav can't be found. r?kentuckyfriedtakahe → MozReview Request: Bug 1254858: P7. Add logging if libmozav can't be found. r=kentuckyfriedtakahe
Assignee

Comment 24

3 years ago
Comment on attachment 8728778 [details]
MozReview Request: Bug 1254858: P8. Change default preferences value if prefs don't exist. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39113/diff/1-2/
Attachment #8728778 - Attachment description: MozReview Request: Bug 1254858: P8. Change default preferences value if prefs don't exist. r?kentuckyfriedtakahe → MozReview Request: Bug 1254858: P8. Change default preferences value if prefs don't exist. r=kentuckyfriedtakahe
Assignee

Comment 25

3 years ago
UL location can't be used as reference as its location varies.

Review commit: https://reviewboard.mozilla.org/r/39159/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39159/
Attachment #8728819 - Flags: review?(mh+mozilla)
Comment on attachment 8728819 [details]
MozReview Request: Bug 1254858: P9. Search libmozav* lib relative to lgpllibs. r=glandium

https://reviewboard.mozilla.org/r/39159/#review35801

::: dom/media/platforms/ffmpeg/ffvpx/FFVPXRuntimeLinker.cpp:57
(Diff revision 1)
> +  char* lgpllibsname = PR_GetLibraryName(nullptr, "lgpllibs");

You should null check lgpllibsname because PR_GetLibraryFilePathname doesn't.
Attachment #8728819 - Flags: review?(mh+mozilla) → review+
Assignee

Comment 27

3 years ago
Comment on attachment 8728359 [details]
MozReview Request: Bug 1254858: P3. reorganise Benchmark classes declarations to only expose usable members. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38919/diff/2-3/
Attachment #8728359 - Attachment description: MozReview Request: Bug 1254858: P3. Add Benchmark::Wait() method to allow for synchronous benchmarking. r=kentuckyfriedtakahe → MozReview Request: Bug 1254858: P3. reorganise Benchmark classes declarations to only expose usable members. r=kentuckyfriedtakahe
Assignee

Comment 28

3 years ago
Comment on attachment 8728360 [details]
MozReview Request: Bug 1254858: P4. Allow to pass mimetype in constructor. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38921/diff/2-3/
Assignee

Comment 29

3 years ago
Comment on attachment 8728361 [details]
MozReview Request: Bug 1254858: P5. Add h264 decoding gtest. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38923/diff/3-4/
Assignee

Comment 30

3 years ago
Comment on attachment 8728776 [details]
MozReview Request: Bug 1254858: P6. Add VP9 decoding gtest. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39109/diff/1-2/
Assignee

Comment 31

3 years ago
Comment on attachment 8728777 [details]
MozReview Request: Bug 1254858: P7. Add logging if libmozav can't be found. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39111/diff/2-3/
Assignee

Comment 32

3 years ago
Comment on attachment 8728778 [details]
MozReview Request: Bug 1254858: P8. Change default preferences value if prefs don't exist. r=kentuckyfriedtakahe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39113/diff/2-3/
Assignee

Comment 33

3 years ago
Comment on attachment 8728819 [details]
MozReview Request: Bug 1254858: P9. Search libmozav* lib relative to lgpllibs. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39159/diff/1-2/
Attachment #8728819 - Attachment description: MozReview Request: Bug 1254858: P9. Search libmozav* lib relative to lgpllibs. r?glandium → MozReview Request: Bug 1254858: P9. Search libmozav* lib relative to lgpllibs. r=glandium
Assignee

Comment 34

3 years ago
https://reviewboard.mozilla.org/r/39159/#review35801

> You should null check lgpllibsname because PR_GetLibraryFilePathname doesn't.

thanks for spotting that.
Assignee

Comment 38

3 years ago
Comment on attachment 8728357 [details]
MozReview Request: Bug 1254858: P1. Remove unused member. r=kentuckyfriedtakahe

Patch request is for all changeset

Approval Request Comment
[Feature/regressing bug #]: 1254858
[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 #8728357 - Flags: approval-mozilla-aurora?
Hi Jean-Yves, this is a pretty big code change. I do not feel it has baked enough to be uplifted to Aurora 47. I would prefer to let this ride the trains despite the clear benefit of improved playback quality. Also wondering how good our automated tests are in this area?
Flags: needinfo?(jyavenard)
Assignee

Comment 40

3 years ago
I'll let Anthony decide.
Flags: needinfo?(jyavenard) → needinfo?(ajones)
Assignee

Comment 41

3 years ago
The main reason I wanted this was to ease uplifting. 

Also this code isn't in the Firefox code. It's just in gtests. It doesn't ship as such.
Assignee

Comment 42

3 years ago
Upon 2nd thoughts, this bugs contains a few fixes to the VP9 benchmark tool. So without it, it's probably safer to to uplift any of the VP9 related patches.
(In reply to Ritu Kothari (:ritu) from comment #39)
> Hi Jean-Yves, this is a pretty big code change. I do not feel it has baked
> enough to be uplifted to Aurora 47. I would prefer to let this ride the
> trains despite the clear benefit of improved playback quality. Also
> wondering how good our automated tests are in this area?

The code being changed is isolated to the benchmark code. This patch also adds unit tests which exercise the benchmark code in question. There is also a lot of value in keeping all the benchmark code and changes together in 47 so we don't end up having to debug it twice.
Flags: needinfo?(ajones)
Comment on attachment 8728357 [details]
MozReview Request: Bug 1254858: P1. Remove unused member. r=kentuckyfriedtakahe

This is needed to enable VP9, (fingers crossed) should be safe and has baked in Nightly for 2 weeks, Aurora47+
Attachment #8728357 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8728358 - Flags: approval-mozilla-aurora+
Attachment #8728359 - Flags: approval-mozilla-aurora+
Attachment #8728360 - Flags: approval-mozilla-aurora+
Attachment #8728361 - Flags: approval-mozilla-aurora+
Attachment #8728776 - Flags: approval-mozilla-aurora+
Attachment #8728777 - Flags: approval-mozilla-aurora+
Attachment #8728778 - Flags: approval-mozilla-aurora+
Attachment #8728819 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.