Closed
Bug 1254858
Opened 9 years ago
Closed 9 years ago
Add media decoder gtest
Categories
(Core :: Audio/Video: Playback, defect)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: jya, Assigned: jya)
References
(Blocks 1 open bug)
Details
Attachments
(9 files)
58 bytes,
text/x-review-board-request
|
ajones
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
ajones
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
ajones
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
ajones
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
ajones
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
ajones
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
ajones
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
ajones
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
glandium
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
This will serve as example for the fuzzing team to decode a local file.
Assignee | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38915/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38915/
Attachment #8728357 -
Flags: review?(ajones)
Assignee | ||
Comment 2•9 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 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38921/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38921/
Attachment #8728360 -
Flags: review?(ajones)
Assignee | ||
Comment 5•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38923/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38923/
Attachment #8728361 -
Flags: review?(ajones)
Assignee | ||
Comment 6•9 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•9 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•9 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•9 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•9 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•9 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 14•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/39109/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39109/
Attachment #8728776 -
Flags: review?(ajones)
Assignee | ||
Comment 15•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/39111/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39111/
Attachment #8728777 -
Flags: review?(ajones)
Assignee | ||
Comment 16•9 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+
Updated•9 years ago
|
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+
Updated•9 years ago
|
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•9 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•9 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•9 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 26•9 years ago
|
||
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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 years ago
|
||
https://reviewboard.mozilla.org/r/39159/#review35801
> You should null check lgpllibsname because PR_GetLibraryFilePathname doesn't.
thanks for spotting that.
Comment 35•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/af351a31c287
https://hg.mozilla.org/integration/mozilla-inbound/rev/d93ab2b1415b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8cdbf668997
https://hg.mozilla.org/integration/mozilla-inbound/rev/81269ce3999d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7900dcbea03
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb100508cf8
https://hg.mozilla.org/integration/mozilla-inbound/rev/509933bb6266
https://hg.mozilla.org/integration/mozilla-inbound/rev/a23635e0c783
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8e90b810fc
Comment 36•9 years ago
|
||
Comment 37•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/af351a31c287
https://hg.mozilla.org/mozilla-central/rev/d93ab2b1415b
https://hg.mozilla.org/mozilla-central/rev/c8cdbf668997
https://hg.mozilla.org/mozilla-central/rev/81269ce3999d
https://hg.mozilla.org/mozilla-central/rev/e7900dcbea03
https://hg.mozilla.org/mozilla-central/rev/ebb100508cf8
https://hg.mozilla.org/mozilla-central/rev/509933bb6266
https://hg.mozilla.org/mozilla-central/rev/a23635e0c783
https://hg.mozilla.org/mozilla-central/rev/ff8e90b810fc
https://hg.mozilla.org/mozilla-central/rev/34193f81ee6c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 38•9 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•9 years ago
|
||
I'll let Anthony decide.
Flags: needinfo?(jyavenard) → needinfo?(ajones)
Assignee | ||
Comment 41•9 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•9 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+
status-firefox47:
--- → affected
Comment 45•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ceec4d5edb0
https://hg.mozilla.org/releases/mozilla-aurora/rev/055262589639
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7020c4cc1c4
https://hg.mozilla.org/releases/mozilla-aurora/rev/78260f138a89
https://hg.mozilla.org/releases/mozilla-aurora/rev/15393c275f93
https://hg.mozilla.org/releases/mozilla-aurora/rev/ed3b972887c0
https://hg.mozilla.org/releases/mozilla-aurora/rev/234a58549e11
https://hg.mozilla.org/releases/mozilla-aurora/rev/b00d99ebc124
https://hg.mozilla.org/releases/mozilla-aurora/rev/b2af7b49ae0a
https://hg.mozilla.org/releases/mozilla-aurora/rev/7dafc9287927
You need to log in
before you can comment on or make changes to this bug.
Description
•