Closed Bug 1254858 Opened 4 years ago Closed 4 years ago

Add media decoder gtest

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(9 files)

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
This will serve as example for the fuzzing team to decode a local file.
Blocks: 1236702
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)
Additionally, reorganise Benchmark classes declaration to only expose usable members.

Review commit: https://reviewboard.mozilla.org/r/38919/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38919/
Attachment #8728359 - Flags: review?(ajones)
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/
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/
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/
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/
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+
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/
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+
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
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
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+
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
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/
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/
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/
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/
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/
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
https://reviewboard.mozilla.org/r/39159/#review35801

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

thanks for spotting that.
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)
I'll let Anthony decide.
Flags: needinfo?(jyavenard) → needinfo?(ajones)
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.
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+
You need to log in before you can comment on or make changes to this bug.