Closed Bug 1254858 Opened 9 years ago Closed 9 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.

Attachment

General

Created:
Updated:
Size: