Closed Bug 1146729 Opened 5 years ago Closed 5 years ago

[FFOS] Enable MP4Reader for MSE and local playback

Categories

(Core :: Audio/Video, defect)

37 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bwu, Assigned: bwu)

References

Details

Attachments

(1 file, 7 obsolete files)

All the decoding pipeline is complete. 
We should be able to enable it for MSE and local playback.
Hardware: x86 → ARM
Blocks: MSE-FxOS
Blocks: 1123603
To enable it, you might want to check power consumption like Bug 1129264.
(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> To enable it, you might want to check power consumption like Bug 1129264.
Yeah. Thanks for the reminder!
MOZ_FMP4 is not supported on ICS, so disable it.
Attachment #8584249 - Flags: review?(mh+mozilla)
Attachment #8582139 - Flags: review?(cpearce)
Attachment #8582139 - Flags: review?(cpearce) → review+
Comment on attachment 8584249 [details] [diff] [review]
Bug-1146729-Disable-MOZ_FMP4-on-ICS.patch

Review of attachment 8584249 [details] [diff] [review]:
-----------------------------------------------------------------

From a build system perspective, this is fine, but that's the type of policy-ish thing that should be reviewed by a relevant-module peer.
Attachment #8584249 - Flags: review?(mh+mozilla)
Attachment #8584249 - Flags: review?(cpearce)
Attachment #8584249 - Flags: review+
Attachment #8584249 - Flags: review?(cpearce) → review+
Carry r+ from glandium and cpearce.
Attachment #8584249 - Attachment is obsolete: true
Attachment #8586602 - Flags: review+
Carry r+ from cpearce.
Attachment #8582139 - Attachment is obsolete: true
Attachment #8586603 - Flags: review+
What about the B2G mochitest failures?
126 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | MediaKeyError should be defined on the global scope - expected PASS
etc..
Keywords: checkin-needed
Depends on: 1129264
Depends on: 1149984
No longer depends on: 1149984
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> What about the B2G mochitest failures?
> 126 INFO TEST-UNEXPECTED-FAIL |
> dom/tests/mochitest/general/test_interfaces.html | MediaKeyError should be
> defined on the global scope - expected PASS
> etc..
Thanks for reminding me to check this again, I thought it is a known failure. 
After checking with jwwang again, MOZ_EME is based on MOZ_FMP4[1]. 
So disabling MOZ_FMP4 will affact MOZ_EME and cause these failures. 

[1]https://dxr.mozilla.org/mozilla-central/source/configure.in#5334
Attachment #8586602 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=068acbbce4f1
Re-run the test cases without applying attachment 8586602 [details] [diff] [review]. 
The testing results look good except [13].
[13] should be a known problem, right? :)
Keywords: checkin-needed
The crash looks new to me which could be caused by this patch...
(In reply to JW Wang [:jwwang] from comment #13)
> The crash looks new to me which could be caused by this patch...
I found other people hit the sample problem as well. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=babb7079551d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dfe5d1bd053

Here comes a question. 
How could I know if the failure is caused my code? Of course, we can check the patch to see if there is any connection to that failure. But are there a better way to know it is a known failure or not? By a list or other way?
I usually run Try twice, one before the patch and the other after the patch. And then check if the orange rate is increasing significantly.
Couldn't you just look at the repo you're pushing on top of rather than expending the resources of redundant Try pushes? i.e. https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&filter-searchStr=emu or https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&filter-searchStr=gaia
B2G ICS emulator debug mochitest-13 started failing on the checkin-needed push that included this patch. I backed this one (and the one from bug 1148179) out for being the likely causes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a48da87b7e0

https://treeherder.mozilla.org/logviewer.html#?job_id=8469225&repo=mozilla-inbound
Flags: needinfo?(bwu)
Got it and thanks!  
I will check the failure again.
Flags: needinfo?(bwu)
Blocks: 1140995
Based on attachment 8586603 [details] [diff] [review], add one more define (MOZ_GONK_MEDIACODEC) check to avoid using MP4Reader on ICS.  
Since MOZ_FMP4 is required in ICS due to MOZ_EME depending on MOZ_FMP4, we need to add that define check.
Attachment #8586603 - Attachment is obsolete: true
Attachment #8590249 - Flags: review?(cpearce)
Comment on attachment 8590249 [details] [diff] [review]
Bug-1146729-FFOS-Enable-MP4Reader-for-MSE-and-local-v2.patch

Review of attachment 8590249 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent.
Attachment #8590249 - Flags: review?(cpearce) → review+
Carry r+ from cpearce.
Attachment #8590249 - Attachment is obsolete: true
Attachment #8590598 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/861b69f6f3b2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Backed out at PBylenga's request for causing bug 1153831.
https://hg.mozilla.org/integration/b2g-inbound/rev/767ea640b2f1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla40 → ---
( side note : https://hg.mozilla.org/integration/mozilla-inbound/rev/14c95291eb98 ) 
backed out of M-i, didn't see the comment until I was forced to refresh this bug and spoke to Ryanvm about it.  

we should do backouts on b2g-inbound for faster results ( ie for the 4 pm build ) for smaller backout sets.
https://hg.mozilla.org/mozilla-central/rev/767ea640b2f1
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Ugh, mc-merge didn't handle that nicely...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla40 → ---
adding qawanted to test the patch for this issue to see if we reproduce bug 1153997 with it applied.
Keywords: qawanted
Rebase attachment 8590598 [details] [diff] [review] to current MC code base.
Attachment #8590598 - Attachment is obsolete: true
Attachment #8596427 - Flags: review?(jwwang)
Attachment #8596427 - Flags: review?(jwwang) → review+
Re-run the try server again. The results look good. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d07ec40e9fc4
The problem in bug 1153831 as comment 27 has been fixed in Bug 1153876.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/61f85e0e1235
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Following up on qawanted request from comment 31 for the state of bug 1153997:
Depends-on bug 1153997 has been verified fixed after following the patch workout here. Clearing qawanted tag.
No longer blocks: 1153997
Flags: needinfo?(ktucker)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
No longer depends on: 1163058
You need to log in before you can comment on or make changes to this bug.