Steps to reproduce: 1. Copy any high resolution .webm clip (1st attachment) to sdcard 2. Open video app and play same clip. Expected behavior: No frame drop should observed also video and audio should be in sync. Actual behavior: Frame drop observed. Video stuck at some point and audio continues to play. Also take a look on 2nd attached recorded clip of same issue. Please find below logs which show the high frame corrupted and dropped count. ------- I/GeckoDump( 1613): Video statistics start I/GeckoDump( 1613): Dimensions: 1280x720 I/GeckoDump( 1613): Complete duration: 02:33 I/GeckoDump( 1613): Start time: 00:00 I/GeckoDump( 1613): End time: 00:34 I/GeckoDump( 1613): Total frames: 1042 I/GeckoDump( 1613): Decoded frames: 534 I/GeckoDump( 1613): Corrupted frames: 508 I/GeckoDump( 1613): Rendered frames: 514 I/GeckoDump( 1613): Dropped frames: 528 I/GeckoDump( 1613): Average rendered FPS: 14.83 I/GeckoDump( 1613): Video statistics end ---------------------

QA Wanted here to see if this happens on 1.3.

This issue reproduces on the 03/20/14 1.3 build. Device: Buri v1.3 MOZ RIL BuildID: 20140320004000 Gaia: 35a51cad2b8cd63c62152bc42c242ce67f27ffea Gecko: ed31cfc75f93 Version: 28.0 Firmware Version: V1.2-device.cfg Additionally, this issue reproduces on 1.1.

mvines - Why is this a blocker for 1.4 if it's already present on 1.1 & 1.3?

We measure ourselves against other OSes running on the same chipset, not older versions of FxOS running on older chipsets. The bar is constantly being raised with every release and one day (hopefully soon!) we'll reach parity with the other players.

Product, This seems to be a new feature ask. Can you please weigh in if we should take this for 1.4?

Neither of these are blocking bugs IMO -- doesn't meet our criteria. However, these are fine bugs to work on as they are being identified. We *need* to move away from marking all things we want to work on as blocking. Chris Inder, This is a candidate for unblocking.

Are you testing on a device with VP8 hardware decoding? It seems unlikely to me that we'll be able to to HD VP8 without hardware accelerated video decoding.

We do have a VP8 hardware decoder available, I suspect that Gecko is not using it. This may stem back to v1.0 days when we couldn't use the VP8 hardware decoder due to pmem limitations. This is no longer the case on JB and KK, so perhaps we need to look at using the VP8 OMX decoder when !ICS. Moving to 1.4+ because on this class of device it is perfectly reasonable for a user to expect smooth high-resolution VP8 playback, especially when other codecs are smooth at the same resolution. However this could be considered a performance optimization and not a "feature" so adjusting the blocking bug accordingly to give more time here.

Clearing ni, Michael provided info.

OK, we need to change DecoderTraits.cpp to use the OMX decoder/reader when the OMX backend supports HWAccel WebM, and fallback to the existing WebMReader when the MediaOMXReader does not support HWAccel WebM.

Hi Eric, Could you help to find an assignee to handle this blocker? Thanks.

(In reply to Ivan Tsay (:ITsay) from comment #12) > Hi Eric, > > Could you help to find an assignee to handle this blocker? Thanks. Product already weighed in that this isn't a blocker. QC needs to raise their concerns directly with our product team here if they would like to consider this for 1.4. (In reply to Michael Vines [:m1] [:evilmachines] (on PTO until 4/6) from comment #9) > Moving to 1.4+ because on this class of device it is perfectly reasonable > for a user to expect smooth high-resolution VP8 playback, especially when > other codecs are smooth at the same resolution. However this could be > considered a performance optimization and not a "feature" so adjusting the > blocking bug accordingly to give more time here. I think that makes a compelling argument to consider this as something we should look into, but that isn't a case to block on the bug. A blocker is something we would absolutely hold the release on against our triage criteria here - https://wiki.mozilla.org/B2G/Triage#Issues_that_Should_Block. In this bug's case, this isn't a performance regression, as this problem has been present in past releases. So in this case we need to explain why the performance optimization is required for 1.4 - is there a partner app desiring high resolution webm support? Is a critical market in 1.4 requiring this? These are example of questions need to answer here to justify blocking here. What I see here is an argument that we should consider implementing this performance enhancement, but not really a critical need why this is required for 1.4. Given this & the fact that product already claimed this wasn't a blocker, I'm moving this to the backlog. If you still disagree, then I'd suggest following up with our product team to discuss this further.

> product already claimed this wasn't a blocker, I'm moving this to the backlog Noming it again for 1.4. Sandip, can you please provide your comments here.

required for madai, so blocking

Since this is back to 1.4+, repeating the comment 12 request for assignment.

(In reply to Milan Sreckovic [:milan] from comment #16) > Since this is back to 1.4+, repeating the comment 12 request for assignment. ok. Blake will look into this.

I am going to change the default decoder of webm files to be stagefright's (OmxDecoder) which will check media_codecs.xml (located at /system/etc/) to use HW decoder or Google's SW decoder (not currently-used decoder).

Blake -- is it going to look for the decoder in the order listed in the file? This solution looks good to me. Adding Diego to get their take on it.

drawing attention to the date of expected fix: no later than 4/21.

(In reply to Inder from comment #19) > Blake -- is it going to look for the decoder in the order listed in the file? > This solution looks good to me. > Adding Diego to get their take on it. SGTM

We might just want to pick the OMX HW decoder if possible and then fall back to the Gecko SW decoder rather than the OMX SW decoder.

(In reply to Inder from comment #19) > Blake -- is it going to look for the decoder in the order listed in the file? Yes. I will not change the existing behavior which already exists in OMXCodec.cpp(http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/OMXCodec.cpp#182). The only files I will change is DecoderTraits.cpp (add webm mime type to OMX decoder supported list)and OmxDecoder.cpp (set the flags to be 0 to not use HW codec only, if there is no HW codec, fall back to SW codec). The patch will be attached today.

(In reply to Michael Vines [:m1] [:evilmachines] from comment #22) > We might just want to pick the OMX HW decoder if possible and then fall back > to the Gecko SW decoder rather than the OMX SW decoder. May I know the concern you prefer not to use OMX SW decoder?

It would be good if we can use the existing WebMReader when we need to fallback to SW decoding of WebM instead of using OMX's SW decoder. We've been been using WebMReader in desktop Firefox for years, so has been tested more.

On B2G, partners may expect to have a consistent behavior/capability with Android version. For OMX SW decoder, it should be tested by many Android users.

(In reply to Blake Wu [:bwu] from comment #26) > On B2G, partners may expect to have a consistent behavior/capability with > Android version. Which partners have this expectation? It's better to use the existing Gecko SW decoder for v1.4 for a couple reasons: 1) Less risk. The OMX SW decoder may be better and it may be worse. We don't really know. 2) Consistency between Firefox OS/Desktop, as well as between FxOSes on different devices. 3) Less blobs for Mozilla. The OMX SW decoder may or may not be something you have source to. Note that on some older devices the OMX HW decoder cannot support the same max resolution as the SW decoder (Gecko or OMX) due to the use of pmem for decoding (Buri is one such device). This bug has the possibility of breaking higher-resolution webm content on those devices. One possible way to avoid this side-effect is to only enable the OMX HW codec for a JB or greater gonk as those devices are all ICS based.

(In reply to Michael Vines [:m1] [:evilmachines] from comment #27) > (In reply to Blake Wu [:bwu] from comment #26) > > On B2G, partners may expect to have a consistent behavior/capability with > > Android version. > > Which partners have this expectation? Phone makers. I think they may compare the Android's version and FFOS's. > It's better to use the existing Gecko SW decoder for v1.4 for a couple > reasons: > 1) Less risk. The OMX SW decoder may be better and it may be worse. We > don't really know. > 2) Consistency between Firefox OS/Desktop, as well as between FxOSes on > different devices. > 3) Less blobs for Mozilla. The OMX SW decoder may or may not be something > you have source to. Agree. Just thought OMX SW decoder may be optimized for mobile device. > > Note that on some older devices the OMX HW decoder cannot support the same > max resolution as the SW decoder (Gecko or OMX) due to the use of pmem for > decoding (Buri is one such device). This bug has the possibility of > breaking higher-resolution webm content on those devices. One possible way > to avoid this side-effect is to only enable the OMX HW codec for a JB or > greater gonk as those devices are all ICS based. Fair enough and also agree these concerns.

I also think it is not a good idea to use androis' sw codec from the following point of view. - To keep consistent behavior between firefox browsers. - Andorid SW codec is not tested on b2g - We did not configure for android's SW codec(like decoded buffer number)

In future, b2g's media platform should be more platform independent except hw codec. platform dependent sw codec needs to be prevented.

BTW, I found in the media_codecs.xml of Nexus4 (https://www.codeaurora.org/cgit/quic/la/device/lge/mako/tree/media_codecs.xml?h=LNX.LA.3.2.1.5#n64) webm is not decoded by QCT HW decoder. After checking the pull-out the media_codecs.xml from Nexus5 with Android version, it doesn't use HW decoder either. Is there any concern not to use HW decoder in these two devices?

Not really a concern for the N5 since that's just a Mozilla reference device. Do you have a Flame or a QRD8x26 device? If not, we can certainly test out a WIP patch over here.

Hi Michael, you can use this patch to have a try to see if HW decoder can be used. I am trying to modify the media_codecs.xml on my Nexus4 and QRD/Flame to see if HW decoder can be accessed.

Pooja, could you please try out this WIP patch.

VP8 HW decoder works well on my QRD with no-change media_codecs.xml which has a HW vp8 decoder in the list. Compared to SW decoder, I can observe the playback is more smooth with HW decoder. But there is no rule to know which device can support vp8 HW decoder (Nexus4 cannot support), unless we query the media_codecs.xml.

What happens if the VP8 HW decoder encounters a WebM file that uses VP9? Will it fallback to our internal VP9 support?

(In reply to cajbir (:cajbir) from comment #36) > What happens if the VP8 HW decoder encounters a WebM file that uses VP9? > Will it fallback to our internal VP9 support? Yes. I expect to do that as well. Besides, some platform may support VP9 HW decoder in the near future. So will check if there is any HW decoder supported or not. If not, fallback to our internal decoder.

Hi All, Currently in Gecko,IIRC we don't have a fallback mechanism to use other decoder when some decoder is not capable of decoding some profile/type well since demuxer is created after newing decoder. For a long-term solution, I think we should change the current design to create a demuxer first and then choose a proper decoder. For a short-term solution, I am going to write/use a lightweight webm file parser to get the codec type(VP8 or VP9) before newing decoder in DecoderTraits::InstantiateDecoder(). If OMX has a HW decoder, use it. Otherwise, use internal decoder. Agree? Other ideas?

With fix high resolution Webm clip is playing very well. Ideally dropped frame count should not exceed 1% of total frames. But with this fix its currently 2% .Is it acceptable ? ---LOGs----- I/GeckoDump( 1498): Video statistics start I/GeckoDump( 1498): Dimensions: 1920x1088 I/GeckoDump( 1498): Complete duration: 00:10 I/GeckoDump( 1498): Start time: 00:00 I/GeckoDump( 1498): End time: 00:00 I/GeckoDump( 1498): Total frames: 732 I/GeckoDump( 1498): Decoded frames: 731 I/GeckoDump( 1498): Corrupted frames: 1 I/GeckoDump( 1498): Rendered frames: 718 I/GeckoDump( 1498): Dropped frames: 14 I/GeckoDump( 1498): Average rendered FPS: 23.09 I/GeckoDump( 1498): Video statistics end ---LOGS------------

(In reply to Blake Wu [:bwu] from comment #38) > For a short-term solution, I am going to write/use a lightweight webm file > parser to get the codec type(VP8 or VP9) before newing decoder in > DecoderTraits::InstantiateDecoder(). If OMX has a HW decoder, use it. > Otherwise, use internal decoder. > Agree? Other ideas? Instead of using your own parser, maybe use libnestegg which is what we use elsewhere for parsing WebM. Or could the mime type sniffer be extended? I'm wary of having half a dozen slightly different lightweight WebM parsers in the tree (sniffer, libnestegg, MSE, WebRTC, etc).

The mime type sniffer uses libnestegg for matching WebM files. It's actually pretty straightforward to use or adapt for your usecases, I'm sure kinetik or me can provide info.

Comment about attachment 8407232 [details] [diff] [review]. We limit hw decoder for video decoding from the following reasons. We should keep that. SW video codec can not provide enough quality as a product. - Can not provide enough decoding performance - android H.264 decoder support only baseline profile. - It could cause oom problem. It actually happened in the past.

I think that if hw video codec is enabled on a format, sw vide codec should not be used for the the format on the product. Otherwise, we can not ensure the product quality.

(In reply to Blake Wu [:bwu][:blakewu] from comment #38) > For a short-term solution, I am going to write/use a lightweight webm file > parser to get the codec type(VP8 or VP9) before newing decoder in > DecoderTraits::InstantiateDecoder(). If there is a good enough parser on open source, it is better to use it on all platforms.

(In reply to Sotaro Ikeda [:sotaro] from comment #43) > I think that if hw video codec is enabled on a format, sw vide codec should > not be used for the the format on the product. Otherwise, we can not ensure > the product quality. From the above point, OMXDecoder does not use SW video decoder on b2g.

(In reply to Pooja from comment #39) > > Ideally dropped frame count should not exceed 1% of total frames. > But with this fix its currently 2% .Is it acceptable ? Before: >50% dropped frames After: ~2% dropped frames Yeah, that'll do for v1.4 :) We can squeeze out that last 1% in v2.0

I'd guess that to reduce that 2% drop more we'd need to change our rendering pipeline so that the compositor decides which frame needs to be displayed on every vertical refresh, rather than us setting a timer and updating the current frame that way. This would be a good change to make, but it's not a simple change...

(In reply to Chris Pearce (:cpearce) from comment #47) > I'd guess that to reduce that 2% drop more we'd need to change our rendering > pipeline so that the compositor decides which frame needs to be displayed on > every vertical refresh, rather than us setting a timer and updating the > current frame that way. This would be a good change to make, but it's not a > simple change... One difficulty is OMXCodec's gralloc buffer is very scarce. If we hold more buffer on Compositor, OMXCodec fall into buffer starvation.

Thanks for Padenot and Kinetik for the information they provided on irc. I am trying to use nestegg sniffer in a newly-added callback used in nsBaseChannel. But hit a problem that there is no simple way to bring the codec id back to HTMLMediaElement(OnStartRequest()) and need your suggestions. The following is my ideas. 1. Create a new callback function(CallWebmSniffer) as CallUnknownTypeSniffer does to sniff the codec id in http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsBaseChannel.cpp#705 ------- if (NS_SUCCEEDED(mStatus) && shouldSniff) { mPump->PeekStream(CallUnknownTypeSniffer, static_cast<nsIChannel*>(this)); } /*newly-added*/ mPump->PeekStream(CallWebmSniffer, static_cast<nsIChannel*>(this));//new // Now, the general type sniffers. Skip this if we have none. if (mLoadFlags & LOAD_CALL_CONTENT_SNIFFERS) mPump->PeekStream(CallTypeSniffers, static_cast<nsIChannel*>(this)); ---- 2. Inside CallWebmSniffer(), I can use nestegg functions to get codec id. But there is no interfaces from nsIChannel to set/store this sniffed information and then get it in HTMLMediaElement, like SetContentType/GetContentType. Do you suggest to add new interface (setContentSubType) or there is another way to bring codec id back to HTMLMediaElement for us to better know to create a proper decoder?

I'm not sure we want to sniff everything that flows through necko, here. We could simply run the logic sniffing logic again when we are about to instantiate a webm decoder.

Yeah, what Paul said. I *think* you want to use an nsIStreamListenerTee to siphon off a copy of the incoming data to run the nestegg sniffer (extended to report codecs) against, after which you can shut the tee down and initialize the appropriate decoder. It might be worth discussing your options with a Necko expert before going ahead, as this isn't really my area.

(In reply to Paul Adenot (:padenot) from comment #50) > I'm not sure we want to sniff everything that flows through necko, here. We > could simply run the logic sniffing logic again when we are about to > instantiate a webm decoder. Could you share me some hints to do this? I have no idea to directly read (from channel?) the media data to do sniffing in the HTMLMediaElement or DecoderTraits. Currently the way I think to reading and sniffing data is from Necko like current getContentType.

(answered on irc)

Hi Patrick, May I have your help on reading stream/data via Necko? Is it possible to read data via the "channel" @http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#340? I need to read the data and do some sniffing to get codec ID, not only getting mime type. Do you have other suggestions, like comment 51?

Blake Can your fix for HW decoder be uplifted and we can take care of the SW piece within another week?

(In reply to Preeti Raghunath(:Preeti) from comment #55) > Blake > > Can your fix for HW decoder be uplifted and we can take care of the SW piece > within another week? OK. I just created a new bug (bug 1000671) for the fallback mechanism. Will enable HW codec first and take enough time on fallback mechanism.

Comment on attachment 8407232 [details] [diff] [review] Enable webm OMX decoder Review of attachment 8407232 [details] [diff] [review]: ----------------------------------------------------------------- Use HW decoder if available, otherwise fallback to omx SW decoder.

Comment on attachment 8407232 [details] [diff] [review] Enable webm OMX decoder Review of attachment 8407232 [details] [diff] [review]: ----------------------------------------------------------------- Software video codec can not ensure h.264's playback capability and quality.

(In reply to Sotaro Ikeda [:sotaro] from comment #58) > Comment on attachment 8407232 [details] [diff] [review] > Enable webm OMX decoder > > Review of attachment 8407232 [details] [diff] [review]: > ----------------------------------------------------------------- > > Software video codec can not ensure h.264's playback capability and quality. video codec's acquire/release happens asynchronously between video tags. Even when each applications use only one video tag, fallback to sw video codec could happen. It actually happened in the past on b2g.

Comment on attachment 8407232 [details] [diff] [review] Enable webm OMX decoder Review of attachment 8407232 [details] [diff] [review]: ----------------------------------------------------------------- Additional comment. ::: content/media/omx/OmxDecoder.cpp @@ +503,5 @@ > > // Experience with OMX codecs is that only the HW decoders are > // worth bothering with, at least on the platforms where this code > // is currently used, and for formats this code is currently used > + // for (h.264). If we don't get a hardware decoder, just fall back to spece on the end of line. @@ +504,5 @@ > // Experience with OMX codecs is that only the HW decoders are > // worth bothering with, at least on the platforms where this code > // is currently used, and for formats this code is currently used > + // for (h.264). If we don't get a hardware decoder, just fall back to > + // other SW decoders specified in media_codecs.xml. OMXCodec does not check media_codecs.xml. Did you expect MediaCodec?

(In reply to Sotaro Ikeda [:sotaro] from comment #59) > > ----------------------------------------------------------------- > > > > Software video codec can not ensure h.264's playback capability and quality. > > video codec's acquire/release happens asynchronously between video tags. > Even when each applications use only one video tag, fallback to sw video > codec could happen. It actually happened in the past on b2g. Sorry, fix the comment. OmxDecoder allocates OMXDecoder via OMXCodecProxy. OMXCodecProxy serializes the video decoder allocation requests. So, the patch should not degrade the H.264 video.

(In reply to Sotaro Ikeda [:sotaro] from comment #60) > OMXCodec does not check media_codecs.xml. Did you expect MediaCodec? IIRC, OMXCodec check media_codecs.xml in findMatchingCodecs() @http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/OMXCodec.cpp#189 Read file @ http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/MediaCodecList.cpp#39

attachment 8407232 [details] [diff] [review] actually does not change using hardware codec for H.264 case on b2g. And instance of OMXDecoder as video decoder is limited to one. If we use OMXCodec for webm video playback, this insatance limitation is also applied to webm video. It change how webm video could be playback. This change seems very big for b2g v1.4 at this time. We need to get a consensus about change.

(In reply to Blake Wu [:bwu][:blakewu] from comment #62) > (In reply to Sotaro Ikeda [:sotaro] from comment #60) > > OMXCodec does not check media_codecs.xml. Did you expect MediaCodec? > IIRC, OMXCodec check media_codecs.xml in findMatchingCodecs() > @http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/ > OMXCodec.cpp#189 > Read file @ > http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/ > MediaCodecList.cpp#39 Ok, thanks.

Comment on attachment 8407232 [details] [diff] [review] Enable webm OMX decoder Review of attachment 8407232 [details] [diff] [review]: ----------------------------------------------------------------- I change to review+ if Comment 63 becomes clear.

(In reply to Blake Wu [:bwu][:blakewu] from comment #62) > (In reply to Sotaro Ikeda [:sotaro] from comment #60) > > OMXCodec does not check media_codecs.xml. Did you expect MediaCodec? > IIRC, OMXCodec check media_codecs.xml in findMatchingCodecs() > @http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/ > OMXCodec.cpp#189 > Read file @ > http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/ > MediaCodecList.cpp#39 It might be better that media_codecs.xml is used by OMXCodec since JB. ICS does not use it.

> It might be better that media_codecs.xml is used by OMXCodec since JB. ICS > does not use it. Correction: It might better to explain that media_codecs.xml is used by OMXCodec since JB.

Comment on attachment 8407232 [details] [diff] [review] Enable webm OMX decoder Review of attachment 8407232 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/DecoderTraits.cpp @@ +214,5 @@ > "video/mp4", > "video/3gpp", > "video/quicktime", > + "video/webm", > + "audio/webm", You also need to change DecoderTraits::CanHandleMediaType(). You need to add code to the IsOmxSupportedType() block that sets codecList to the WebM codecs supported by the OMX decoder if IsWebMType(nsDependentCString(aMIMEType)) is true.

By the way, iirc, webm's parser had some defects in past android. I am not sure about current android. It seems necessary to do a lot of testing of webm after enabling it.

Is it good to refine the naming of MediaResourceManagerClient::HW_VIDEO_DECODER since OMXCodecProxy doesn't limit to allocate hardware-only video decoders any more? - http://dxr.mozilla.org/mozilla-central/source/content/media/omx/OMXCodecProxy.cpp#129

(In reply to Sotaro Ikeda [:sotaro] from comment #63) > attachment 8407232 [details] [diff] [review] actually does not change using > hardware codec for H.264 case on b2g. And instance of OMXDecoder as video > decoder is limited to one. > > If we use OMXCodec for webm video playback, this insatance limitation is > also applied to webm video. It change how webm video could be playback. This > change seems very big for b2g v1.4 at this time. We need to get a consensus > about change. If I understand correctly, you mean webm's behavior (instance limitation) will be like current h.264, right?

(In reply to Bruce Sun [:brsun] from comment #70) > Is it good to refine the naming of > MediaResourceManagerClient::HW_VIDEO_DECODER since OMXCodecProxy doesn't > limit to allocate hardware-only video decoders any more? > - > http://dxr.mozilla.org/mozilla-central/source/content/media/omx/ > OMXCodecProxy.cpp#129 Yeah, it seems better to change the name to more generic one.

(In reply to Blake Wu [:bwu][:blakewu] from comment #71) > (In reply to Sotaro Ikeda [:sotaro] from comment #63) > > attachment 8407232 [details] [diff] [review] actually does not change using > > hardware codec for H.264 case on b2g. And instance of OMXDecoder as video > > decoder is limited to one. > > > > If we use OMXCodec for webm video playback, this insatance limitation is > > also applied to webm video. It change how webm video could be playback. This > > change seems very big for b2g v1.4 at this time. We need to get a consensus > > about change. > If I understand correctly, you mean webm's behavior (instance limitation) > will be like current h.264, right? Yes. It seems better for b2g. Almost all b2g phone have a very tight memory/cpu resource limitations. But it might affect to some existing testing or use cases.

1. Add a OMX webm codeclist for CanHandleMediaType 2. Add description for media_codecs.xml supported since JB

Comment on attachment 8412292 [details] Enable webm OMX decoder with CanHandleMediaType Obsolete this and re-submit a new one.

Comment on attachment 8412337 [details] [diff] [review] Enable webm OMX decoder with CanHandleMediaType Review of attachment 8412337 [details] [diff] [review]: ----------------------------------------------------------------- I think we should have a pref to enable this. The pref should be off by default, and only enabled where we think we need it. ::: content/media/DecoderTraits.cpp @@ +246,5 @@ > nullptr > }; > + > +static char const *const gOMXWebMCodecs[4] = { > + "vorbis", Don't add trailing whitespace, on the "vorbis" and "vp8" lines. @@ +409,5 @@ > result = CANPLAY_MAYBE; > if (nsDependentCString(aMIMEType).EqualsASCII("audio/mpeg")) { > codecList = gMpegAudioCodecs; > + } else if (nsDependentCString(aMIMEType).EqualsASCII("audio/webm")|| > + nsDependentCString(aMIMEType).EqualsASCII("video/webm")) { } else if (nsDependentCString(aMIMEType).EqualsASCII("audio/webm") || nsDependentCString(aMIMEType).EqualsASCII("video/webm")) { i.e. space between ')' and "||", and line up conditions in if statements.

How about the pref off for ICS based devices but enabled for KK based devices? We'd always want to use this for KK (until we don't, and then we can file a new bug for that)

Comment on attachment 8412337 [details] [diff] [review] Enable webm OMX decoder with CanHandleMediaType Review of attachment 8412337 [details] [diff] [review]: ----------------------------------------------------------------- Review only OmxDecoder part. Looks good. Comment 78 seems reasonable comment. We do not need to change ICS gonk's capability in this version. ::: content/media/omx/OmxDecoder.cpp @@ +504,5 @@ > // Experience with OMX codecs is that only the HW decoders are > // worth bothering with, at least on the platforms where this code > // is currently used, and for formats this code is currently used > + // for (h.264). So if we don't get a hardware decoder, just fallback > + // to sw decoders which are listed in MediaCodec.xml since JB Remove space @@ +505,5 @@ > // worth bothering with, at least on the platforms where this code > // is currently used, and for formats this code is currently used > + // for (h.264). So if we don't get a hardware decoder, just fallback > + // to sw decoders which are listed in MediaCodec.xml since JB > + int flags = 0;//kHardwareCodecsOnly; It seems better to remove commented out "kHardwareCodecsOnly".

Thanks for all your feedbacks! Also agree to add a pref to be more flexible to enable on some version or device. The following is what I am going to do. 1. Add a pref and enable it on by default KK as comment 77, 78, and 79. 2. Modify the current test case to check vp9 and opus according the pref. 3. Remove some unnecessary whitespace and comment and line up conditions as comment 77 and 79. Please let me if there is anything missed.

(In reply to Blake Wu [:bwu][:blakewu] from comment #80) > Thanks for all your feedbacks! > Also agree to add a pref to be more flexible to enable on some version or > device. > > The following is what I am going to do. > 1. Add a pref and enable it on by default KK as comment 77, 78, and 79. > 2. Modify the current test case to check vp9 and opus according the pref. > 3. Remove some unnecessary whitespace and comment and line up conditions as > comment 77 and 79. > Please let me if there is anything missed. Blake, Are you creating another patch which addresses all of the points in comment #80?

I enable omx webm decoder on JB as well since currently Flame has not had KK yet but it has webm HW decoder supported.

(In reply to Preeti Raghunath(:Preeti) from comment #81) > Blake, > > Are you creating another patch which addresses all of the points in comment > #80? Yes. But since Flame which supports webm hw decoder still uses JB, I enable omx webm decoder on JB as well. For the test case, I am modifying it and submit another patch later.

Comment on attachment 8414171 [details] [diff] [review] enable-omx-webm-decoder-jb-kk Review of attachment 8414171 [details] [diff] [review]: ----------------------------------------------------------------- r=cpearce for DecoderTraits.cpp. sotaro needs to review OmxDecoder.cpp You need a build peer (like :glandium) to review configure.in. ::: configure.in @@ +259,5 @@ > AC_SUBST(MOZ_OMX_DECODER) > MOZ_OMX_ENCODER=1 > AC_SUBST(MOZ_OMX_ENCODER) > AC_DEFINE(MOZ_OMX_ENCODER) > + MOZ_OMX_WEBM_DECODER=1 You need to get review from a build peer on this. Like Mike Hommey (:glandium). ::: content/media/DecoderTraits.cpp @@ +206,5 @@ > } > #endif > > #ifdef MOZ_OMX_DECODER > +#ifdef MOZ_OMX_WEBM_DECODER I think you can make this: ifdef MOZ_OMX_DECODER static const char* const gOmxTypes[] = { "audio/mpeg", "audio/mp4", "audio/amr", "video/mp4", "video/3gpp", "video/quicktime", #ifdef MOZ_OMX_WEBM_DECODER "video/webm", "audio/webm", #endif nullptr }; @@ +422,5 @@ > if (IsOmxSupportedType(nsDependentCString(aMIMEType))) { > result = CANPLAY_MAYBE; > if (nsDependentCString(aMIMEType).EqualsASCII("audio/mpeg")) { > codecList = gMpegAudioCodecs; > +#ifdef MOZ_OMX_WEBM_DECODER Please also make the MOZ_WEBM block inside CanHandleMediaType defined out if MOZ_OMX_WEBM_DECODER is defined, i.e.: #if defined(MOZ_WEBM) && !defined(MOZ_OMX_WEBM_DECODER) if (IsWebMType(nsDependentCString(aMIMEType))) { codecList = gWebMCodecs; result = CANPLAY_MAYBE; } #endif

(In reply to Chris Pearce (:cpearce) from comment #84) > Comment on attachment 8414171 [details] [diff] [review] > enable-omx-webm-decoder-jb-kk > > Review of attachment 8414171 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=cpearce for DecoderTraits.cpp. > > sotaro needs to review OmxDecoder.cpp > > You need a build peer (like :glandium) to review configure.in. Will do! > ::: configure.in > @@ +259,5 @@ > > AC_SUBST(MOZ_OMX_DECODER) > > MOZ_OMX_ENCODER=1 > > AC_SUBST(MOZ_OMX_ENCODER) > > AC_DEFINE(MOZ_OMX_ENCODER) > > + MOZ_OMX_WEBM_DECODER=1 > > You need to get review from a build peer on this. Like Mike Hommey > (:glandium). > > ::: content/media/DecoderTraits.cpp > @@ +206,5 @@ > > } > > #endif > > > > #ifdef MOZ_OMX_DECODER > > +#ifdef MOZ_OMX_WEBM_DECODER > > I think you can make this: Yes. It is much simpler. > ifdef MOZ_OMX_DECODER > static const char* const gOmxTypes[] = { > "audio/mpeg", > "audio/mp4", > "audio/amr", > "video/mp4", > "video/3gpp", > "video/quicktime", > #ifdef MOZ_OMX_WEBM_DECODER > "video/webm", > "audio/webm", > #endif > nullptr > }; > > @@ +422,5 @@ > > if (IsOmxSupportedType(nsDependentCString(aMIMEType))) { > > result = CANPLAY_MAYBE; > > if (nsDependentCString(aMIMEType).EqualsASCII("audio/mpeg")) { > > codecList = gMpegAudioCodecs; > > +#ifdef MOZ_OMX_WEBM_DECODER > > Please also make the MOZ_WEBM block inside CanHandleMediaType defined out if > MOZ_OMX_WEBM_DECODER is defined, i.e.: OK. To be mutually exclusive with internal webm decoder > #if defined(MOZ_WEBM) && !defined(MOZ_OMX_WEBM_DECODER) > if (IsWebMType(nsDependentCString(aMIMEType))) { > codecList = gWebMCodecs; > result = CANPLAY_MAYBE; > } > #endif

Add a define of MOZ_OMX_WEBM_DECODER for JB and KK only. DecoderTraits and OmxDecoder check it to enable OMX WebM decoder.

Changes in DecoderTraits.cpp 1. Check MOZ_OMX_WEBM_DECODER to see if it is required to use omx webm decoder or not. 2. Re-summit with some changes mentioned in comment 88.

Changes in OmxDecoder.cpp. Allow to use OMX SW decoder if there is no HW decoder supported.

(In reply to Blake Wu [:bwu][:blakewu] from comment #87) > Created attachment 8414362 [details] [diff] [review] > add-omx-webm-decoder-jb-kk > > Changes in DecoderTraits.cpp > 1. Check MOZ_OMX_WEBM_DECODER to see if it is required to use omx webm > decoder or not. opps! some typos. Corrected it as below. 2. Re-submit with some changes mentioned in comment 84.

Test case change in can_play_type_webm.js. Don't check vp9 and opus when OMX webm decoder enabled in JB and KK (Android Version > 15).

Comment on attachment 8414358 [details] [diff] [review] Add a define for omx webm decoder on JB and KK Review of attachment 8414358 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +259,5 @@ > AC_SUBST(MOZ_OMX_DECODER) > MOZ_OMX_ENCODER=1 > AC_SUBST(MOZ_OMX_ENCODER) > AC_DEFINE(MOZ_OMX_ENCODER) > + MOZ_OMX_WEBM_DECODER=1 This line is not needed. @@ +260,5 @@ > MOZ_OMX_ENCODER=1 > AC_SUBST(MOZ_OMX_ENCODER) > AC_DEFINE(MOZ_OMX_ENCODER) > + MOZ_OMX_WEBM_DECODER=1 > + AC_DEFINE(MOZ_OMX_WEBM_DECODER) Do you need that set on all the tree?

(In reply to Mike Hommey [:glandium] from comment #91) > Comment on attachment 8414358 [details] [diff] [review] > Add a define for omx webm decoder on JB and KK > > Review of attachment 8414358 [details] [diff] [review]: > ----------------------------------------------------------------- > Do you need that set on all the tree? Thanks for your hint. It is no need to add this in configure.in. I will add it to the corresponding moz.build.

Add MOZ_OMX_WEBM_DECODER define in moz.builds in the gecko/content/media and gecko/contenet/media/omx to check if it is required to enable omx webm decoder or not.

Comment on attachment 8414407 [details] [diff] [review] Modify the test case of can_play_webm_type Review of attachment 8414407 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/test/can_play_type_webm.js @@ +19,5 @@ > + var audio = ['vorbis']; > + } else { > + var video = ['vp8', 'vp8.0', 'vp9', 'vp9.0']; > + var audio = ['vorbis', 'opus']; > + } Put spaces around operators, like '+' and '>', and on the outside of parenthesis ('(' and ')') in "if" statements. So that should be: if (androidVer > 15) { var video = ['vp8', 'vp8.0']; var audio = ['vorbis']; } else { var video = ['vp8', 'vp8.0', 'vp9', 'vp9.0']; var audio = ['vorbis', 'opus']; }

Update the attachment 8414407 [details] [diff] [review] for the followings. 1. Add the OS check for B2G 2. Put spaces around operators

Comment on attachment 8417260 [details] [diff] [review] Modify the test case of can_play_type_webm v2 Review of attachment 8417260 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/test/can_play_type_webm.js @@ +6,5 @@ > // WebM types > check("video/webm", "maybe"); > check("audio/webm", "maybe"); > > + // Enable OMX webm decoder after Android version 16 as MOZ_OMX_WEBM_DECODER You comment here says that you are enabling OMX WebM decoder. But this code does not do that. The code detects whether the OMX WebM decoder is enabled. The comment needs to be changed so it's not misleading. @@ +13,5 @@ > + var androidVer = SpecialPowers.Cc['@mozilla.org/system-info;1'] > + .getService(SpecialPowers.Ci.nsIPropertyBag2) > + .getProperty('version'); > + info("android version:"+androidVer); > + if (navigator.userAgent.indexOf("Mobile") != -1 && androidVer > 15) { Won't this disable VP9 and Opus for Android builds? We should keep testing that.

(In reply to Chris Pearce (:cpearce) from comment #96) > Comment on attachment 8417260 [details] [diff] [review] > Modify the test case of can_play_type_webm v2 > > @@ +13,5 @@ > > + var androidVer = SpecialPowers.Cc['@mozilla.org/system-info;1'] > > + .getService(SpecialPowers.Ci.nsIPropertyBag2) > > + .getProperty('version'); > > + info("android version:"+androidVer); > > + if (navigator.userAgent.indexOf("Mobile") != -1 && androidVer > 15) { > > Won't this disable VP9 and Opus for Android builds? We should keep testing > that. AFAIK, the string, "Mobile", is for B2G. This check just keeps that only B2G will be affected. Others will keep the same as usual.

Per comment 96, modify the comment to be more clear and add one more check to make sure this change is only for B2G.

The results of test cases on TBPL. https://tbpl.mozilla.org/?tree=Try&rev=b07ad47535eb

Comment on attachment 8418453 [details] [diff] [review] Modify-test-case-can_play_webm_type-v3 Review of attachment 8418453 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/test/can_play_type_webm.js @@ +6,5 @@ > // WebM types > check("video/webm", "maybe"); > check("audio/webm", "maybe"); > > + // OMX webm decoder is enabled after Android version 16 as MOZ_OMX_WEBM_DECODER This comment says what is happening. The code also says what's happening. So comments that just say what is happening don't add much information (unless the code is very complex and hard to understand). Good comments will say *why* the code is doing what it's doing. So make this comment: // OMX webm decoder is enabled on B2G after Android version 16 (JellyBean) so that we can use hardware acceleration to speed up VP8 video decoding. OMX does not support VP9 or Opus, so we don't test for that when decoding WebM using OMX here. @@ +15,5 @@ > + .getProperty('version'); > + info("android version:"+androidVer); > + //Check for FxOS case > + if (navigator.userAgent.indexOf("Mobile") != -1 && > + navigator.userAgent.indexOf("Android") == -1 && androidVer > 15) { Remove trailing space on these two lines.

Per comment 100, make comments to be more clear :) and remove some unnecessary space.

Add reviewer to patch.

Add reviewer to patch.

Add reviewer to patch.

Add reviewer to patch.

Test results on TBPL: https://tbpl.mozilla.org/?tree=Try&rev=38a2ee17a3ef Check-in needed: Add-OMX-WebM-Decoder-Def.patch: r=glandium Add-omx-webm-decoder-jb-kk.patch: r=cpearce Add-omx-webm-decoder-jb-kk2.patch: r=sotaro Modify-test-case-can_play_webm_type.patch: r=cpearce

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8b0bf673c5 https://hg.mozilla.org/integration/mozilla-inbound/rev/49831d474b17 https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a9fef81547 https://hg.mozilla.org/integration/mozilla-inbound/rev/86616594f0c9

https://hg.mozilla.org/mozilla-central/rev/6d8b0bf673c5 https://hg.mozilla.org/mozilla-central/rev/49831d474b17 https://hg.mozilla.org/mozilla-central/rev/a2a9fef81547 https://hg.mozilla.org/mozilla-central/rev/86616594f0c9

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/34b821bbc7ba https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/b7f5d141c237 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/3ecdbb73c057 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/e96076a30b09

Test case is valid because test pass requires you to play a .webm video. https://moztrap.mozilla.org/manage/case/8582/

Test case updated in moztrap to include paying attention to the framerate when playing .webm video: https://moztrap.mozilla.org/manage/case/8582/