821524 - Add support for using the stagefright HW vp8 decoder

Status: RESOLVED DUPLICATE of bug 986381

(Core :: Audio/Video, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This should be a tiny patch, we just need to add vp8 to the list of codecs that omx advertises.

The issues are
 - the HW decoders are not playing well with us right now, so we should land pref'd off
 - if we can't allocate a HW decoder, we should fall back on the gecko SW decoder since it's guaranteed to be more up to date than stagefright's.  I think this might be tricky in the current code.

Comment 1

[Inder]

Chris, I am looking at enabling the hw decoder for webm files because the current use software decoder causes frame skips. I added the "video/webm" mime type to gOmxTypes. I am noticing errors and crash coming from matroska parser. I am pretty sure the error is because of MediaStreamSource in OmxDecoder.cpp because the same content plays perfectly fine in Android which uses FileSource. As of now I am not sure what exactly is the issue, still looking.

Comment 2

[Inder]

Attachment: Read Error 1

Comment 3

[Inder]

Attachment: Read Error 2

Comment 4

[Inder]

Attachment: Read Error 3

Comment 5

[Michael Vines [:m1] [:evilmachines]]

(Resolving for tef might be real nice as currently we see pretty bad UX with the sw vp8 decoder.  Not setting tef? yet because not sure I have a good understanding of how frequently we could expect to see this format in the wild, anybody have input here?)

Comment 6

[Inder]

After debugging some more and collecting bunch of logs I could confirm that there is indeed an issue with readAt() function in MediaStreamSource implementation.
I compared every data read in ReadUInt() external/libvpx/mkvparser/mkvparser.cpp in FFOS with one in Android and in the 3 attachements you can see how in FFOS some of values read are bad and leads to errors/crash.
Interestingly, the read error doesn't happen at the same instance but it does happen with all webm files I have tested after enabling hw decoder.
To reproduce the issue all you need to do is add "video/webm" in omx mime types list as mentioned in comment 1 and try to play the file. You should see either an assertion crash or the following parse error:
E/MatroskaExtractor(  428): Cluster::Parse returned result -2

Chris, please take a look and let me know if you want me to create a separate bug to get the readAt issue fixed.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Not my area of expertise; trying another Chris ;) .)

Comment 8

[cajbir (:cajbir)]

I can't check until next week unfortunately as Monday is a public holiday in NZ and I've lent the only device I have for testing for an App day in another part of the country, sorry.

I'm curious what the issue is for software decoding of WebM using our built in decoder is though. All videos I've tried have been good. 

Does the video you are attempting to play work ok in Android on the device using the hardware decoder? Do you have an URL so I can test?

Comment 9

[Inder]

> I'm curious what the issue is for software decoding of WebM using our built
> in decoder is though. All videos I've tried have been good. 

Chris, we are seeing occasional frame skips with software codec. Our test guys initially reported it and I could reproduce it with multiple videos.

> 
> Does the video you are attempting to play work ok in Android on the device
> using the hardware decoder? Do you have an URL so I can test?

Yes, on Android (which uses hw codec) the same webm videos plays perfectly fine. The attachments I shared earlier shows the errors we are seeing in webm file reads  on FFOS compared to Android.
This is the video I have been testing with so lets start with this:
https://docs.google.com/uc?export=download&id=0B1o1jCbrNLXoUF8tajJFOUl1dkE

Comment 10

[Inder]

Chris, any update here?

Comment 11

[cajbir (:cajbir)]

(In reply to Inder from comment #10)
> Chris, any update here?

I'm unable to look into this until I get my B2G device back to test with, sorry. Should be today/tomorrow sometime.

Comment 12

[cajbir (:cajbir)]

Where is the patch to enable the VP8 hardware decoder so I can test?

Comment 13

[Inder]

Attachment: Enable HW decode of webm file.

Chris, here the change to enable hw decode of wemb

Comment 14

[cajbir (:cajbir)]

I see no errors playing the video in comment 9 using the hardware decoder on an Otoro device. The video plays well.

Comment 15

[Dylan Oliver [:doliver]]

Chris, any other ideas here? It sounds like this one is still an issue for partners.

Comment 16

[cajbir (:cajbir)]

I can't reproduce the issue on my Otoro. Videos play well using the VP8 hardware decoder. Is it still happening for the original submitter? If so, need device specifics, are they using the same software as the otoro, etc.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Minusing for tracking based on comment 16 not reproducing on Otoro.

Comment 18

[Inder]

Comment on attachment 707346 [details]
Enable HW decode of webm file.

Hmm.. Can't reproduce the read issue in comment 6 anymore. We can go ahead with mainlining the hw decode of webm clips.

Comment 19

[Michael Vines [:m1] [:evilmachines]]

tef? again, for the landing of enabling HW decode of webm.  Would hate for that Si to go to waste.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Has any testing been done of the SW fallback?  If it's not the same quality as the in-gecko decoder, then I suggest we don't land this.  Or rejigger things so we can fall back on the gecko decoder.

Breaking webm would "break the web", which we can't do.  (h264 isn't "part of the web" yet, though almost there.)

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Needed for competitive parity in power usage.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

We want to approve this but need to hear that the fallback testing has been done first so that this doesn't get landed without confirmation that it doesn't break webm.

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 707346 [details]
Enable HW decode of webm file.

(For future reference, this isn't my area of code, so no need to ask me for review if you've already asked doublec :).)

Comment 24

[cajbir (:cajbir)]

(In reply to Lukas Blakk [:lsblakk] from comment #22)
> We want to approve this but need to hear that the fallback testing has been
> done first so that this doesn't get landed without confirmation that it
> doesn't break webm.

This switches away from using our internal WebM playback support to that provided by the device manufacturer. Once we do that we can't really control whether webm is broken or not since it depends on the particular devices implementation.

We don't have support for falling back to our internal WebM playback if the stagefright hardware decoding fails - that will instead fallback to any software implementation that stagefright has. Falling back to our internal decoder isn't trivial and would require some investigation to see what effort is involved.

Comment 25

[Michael Vines [:m1] [:evilmachines]]

FYI we've disabled the stagefright software codecs: 
http://mxr.mozilla.org/mozilla-b2g18/source/content/media/omx/OmxDecoder.cpp#247

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Ugh, yeah.  So we'd have to make that conditional, and test when it's hit or add code to fall back to gecko if the sw decoder is trash.  Not feelin' this one.

There's unused Si in Shit too, but the stuff around it is what keeps people away :/.  Always v.next.

Comment 27

[Michael Vines [:m1] [:evilmachines]]

Ugh.  Forgot that we don't have a fallback to Gecko sw decoders in v1.0.1 once we arrive in OMXDecoder.cpp.   Clearing tef?, this is clearly out.

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Based on comment 24 this looks too risky to track and potentially take without seeing an uplift nomination. If a low risk, well tested option is available please nominate.

Comment 29

[Sotaro Ikeda [:sotaro]]

It is better to re-think about it.

Comment 30

[Preeti Raghunath(:Preeti)]

Sotaro,

The issue sis not land yet. Please discuss risk analysis for it to be made blocker?

Comment 31

[Sotaro Ikeda [:sotaro]]

I clear the blocker of koi. It is too late to land for v1.2.