1146729 - [FFOS] Enable MP4Reader for MSE and local playback

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Blake Wu [:bwu][:blakewu]]

All the decoding pipeline is complete. 
We should be able to enable it for MSE and local playback.

Comment 1

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1146729-FFOS-Enable-MP4Reader-for-MSE-and-local-playback.patch

As comment 0 said.

Comment 2

[Blake Wu [:bwu][:blakewu]]

Try Server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9009f90e890
KK Test Case: https://treeherder.allizom.org/#/jobs?repo=try&revision=a9009f90e890

Comment 3

[Sotaro Ikeda [:sotaro]]

To enable it, you might want to check power consumption like Bug 1129264.

Comment 4

[Blake Wu [:bwu][:blakewu]]

(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!

Comment 5

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1146729-Disable-MOZ_FMP4-on-ICS.patch

MOZ_FMP4 is not supported on ICS, so disable it.

Comment 6

[Mike Hommey [:glandium]]

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.

Comment 7

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1146729-Disable-MOZ_FMP4-on-ICS-final.patch

Carry r+ from glandium and cpearce.

Comment 8

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1146729-FFOS-Enable-MP4Reader-for-MSE-and-local-playback-final.patch

Carry r+ from cpearce.

Comment 9

[Blake Wu [:bwu][:blakewu]]

Testing results look good. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f06c4b5ed6c5

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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..

Comment 11

[Blake Wu [:bwu][:blakewu]]

(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

Comment 12

[Blake Wu [:bwu][:blakewu]]

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? :)

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

The crash looks new to me which could be caused by this patch...

Comment 14

[Blake Wu [:bwu][:blakewu]]

(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?

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a186400dd8

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 19

[Blake Wu [:bwu][:blakewu]]

Got it and thanks!  
I will check the failure again.

Comment 20

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1146729-FFOS-Enable-MP4Reader-for-MSE-and-local-v2.patch

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.

Comment 21

[Blake Wu [:bwu][:blakewu]]

TryServer: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bab43989ab49

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 23

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1146729-FFOS-Enable-MP4Reader-for-MSE-and-local-Final.patch

Carry r+ from cpearce.

Comment 24

[Blake Wu [:bwu][:blakewu]]

TryServer: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bab43989ab49 (the same as comment 21)

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/861b69f6f3b2

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/861b69f6f3b2

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out at PBylenga's request for causing bug 1153831.
https://hg.mozilla.org/integration/b2g-inbound/rev/767ea640b2f1

Comment 28

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

( 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.

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/767ea640b2f1

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Ugh, mc-merge didn't handle that nicely...

Comment 31

[Peter Bylenga [:PBylenga]]

adding qawanted to test the patch for this issue to see if we reproduce bug 1153997 with it applied.

Comment 32

[Blake Wu [:bwu][:blakewu]]

Attachment: Update-Bug-1146729-FFOS-Enable-MP4Reader-for-MSE-and-local-playback.patch

Rebase attachment 8590598 [details] [diff] [review] to current MC code base.

Comment 33

[Blake Wu [:bwu][:blakewu]]

Attachment: Update-Bug-1146729-FFOS-Enable-MP4Reader-for-MSE-and-local-playback-hg.patch

1. Merge attachment 8590598 [details] [diff] [review] and attachment 8596427 [details] [diff] [review]
2. Carry r+ from cpearce and jwwang.

Comment 34

[Blake Wu [:bwu][:blakewu]]

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.

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61f85e0e1235

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61f85e0e1235

Comment 37

[Oliver Nelson [:oliverthor]]

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.