Last Comment Bug 787228 - Enable use of hardware H.264/AAC/MP3 decoders in Android libstagefright omx plugin for GB
: Enable use of hardware H.264/AAC/MP3 decoders in Android libstagefright omx p...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: ARM Android
: -- normal with 1 vote (vote)
: mozilla20
Assigned To: cajbir (:cajbir)
:
Mentors:
Depends on: 787229 829454
Blocks: 755364 817868 820236 805700 823253
  Show dependency treegraph
 
Reported: 2012-08-30 14:50 PDT by Maire Reavy [:mreavy] (On PTO until July 5th)
Modified: 2016-02-06 03:53 PST (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
work in progress patch (545.01 KB, patch)
2012-10-10 17:55 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
work in progress patch (544.71 KB, patch)
2012-10-11 00:05 PDT, cajbir (:cajbir)
no flags Details | Diff | Review
Part 1: content/media/plugin changes (4.99 KB, patch)
2012-11-25 19:41 PST, cajbir (:cajbir)
no flags Details | Diff | Review
Part1: Media plugin backend changes for Gingerbread support (6.13 KB, patch)
2012-11-25 20:07 PST, cajbir (:cajbir)
cpeterson: review-
Details | Diff | Review
Part2: Add libomxplugin support for Gingerbread (11.85 KB, patch)
2012-11-25 20:07 PST, cajbir (:cajbir)
cpeterson: review-
Details | Diff | Review
Part3: Add Android OS headers for Gingerbread (481.27 KB, patch)
2012-11-25 20:08 PST, cajbir (:cajbir)
cpeterson: review+
Details | Diff | Review
Part4: Add libstagefright stub libraries for Gingerbread (11.20 KB, patch)
2012-11-25 20:09 PST, cajbir (:cajbir)
cpeterson: review+
Details | Diff | Review
Part5: Package gingerbread libomxplugin libraries on Android (1.01 KB, patch)
2012-11-25 20:11 PST, cajbir (:cajbir)
mark.finkle: review+
Details | Diff | Review
Part6: Build changes for gingerbread libomxplugin libraries on Android (3.19 KB, patch)
2012-11-25 20:14 PST, cajbir (:cajbir)
khuey: review+
Details | Diff | Review
Part7: Add Android OS Release version to nsSystemInfo (1.49 KB, patch)
2012-11-25 20:16 PST, cajbir (:cajbir)
dougt: review+
Details | Diff | Review
Part1: Media plugin backend changes for Gingerbread support (6.24 KB, patch)
2012-11-26 20:05 PST, cajbir (:cajbir)
cpeterson: review+
Details | Diff | Review
Part2: Add libomxplugin support for Gingerbread (11.52 KB, patch)
2012-11-26 20:08 PST, cajbir (:cajbir)
cpeterson: review+
Details | Diff | Review

Description Maire Reavy [:mreavy] (On PTO until July 5th) 2012-08-30 14:50:17 PDT
Bug 782508 added support for the hardware decoders accessable via libstagefright for Android ICS/JB.  This adds hardware decoding support for GB (Gingerbread).
Comment 1 cajbir (:cajbir) 2012-10-10 17:55:08 PDT
Created attachment 670203 [details] [diff] [review]
work in progress patch

This is my work in progress patch for gingerbread support. It is hacked to load the gingerbread omx plugin so will only run on a GB device.

On the only GB device I have for testing (an otoro device running GB) the shared library loads  but the MediaStreamSource object has incorrect data in its method arguments when the methods are called. I have some basic logging that shows this.

If I use DataSource::CreateFromURI with a hard coded URL then the video in that URL plays fine. This seems to indicate something about the DataSource layout is incorrect since our own derived classes don't work but the built in derived classes do. It'd be great if someone with another GB device can try this and see if they see the same to ensure it's not some otoro weirdness.

The code is a wip and needs finishing - I'm still working on it. Notably the XOmxPlugin.cpp duplication will go away.
Comment 2 cajbir (:cajbir) 2012-10-11 00:05:46 PDT
Created attachment 670278 [details] [diff] [review]
work in progress patch

This fixes the DataSource issue and gives playback of video/audio on GB devices. I still need to tidy it up for review but I've uploaded so people can try on various devices. The patch is still hardcoded to load the GB plugin so won't work on ICS devices.

I've tested on GB otoro. I get these warnings when playing http://cd.pn/b2 on logcat:

E/NativeBuffer(  915): Error translating color format. Defaulting to YUV420 semi planar

Color's are a little off. If I play http://cd.pn/b (which plays fine) then play http://cd.pn/b2 I get no video image but audio plays. Possibly a color format issue.

On the Samsung Galaxy Y, the other GB device I'm using for testing, I get no video as well.

Both these devices are <1 Ghz single core so playback has frame skipping.
Comment 3 Maire Reavy [:mreavy] (On PTO until July 5th) 2012-10-11 11:24:37 PDT
What would be a good GB device for Chris Double to use in his testing?  And if he doesn't have it (which he probably doesn't), can we get it to him quickly?

(For reference: Chris D has been using the Samsung Galaxy Y and the Otoro running GB.  Is either of them sufficient for development purposes, or would an alternative device be better?)

I have the same question regarding a good HC (Honeycomb) device, but I can ask that in a separate bug.  Thanks.
Comment 4 Chris Peterson [:cpeterson] 2012-10-11 11:53:06 PDT
Maire, here is a rough list of devices that are most popular with our Firefox users (from our Google Play Store stats).

The Galaxy S II (GT-I9100 model) and Galaxy Note (GT-N7000 model) are popular devices that shipped with GB (but offer an optional upgrade to ICS). The HTC Desire HD and Galaxy S are somewhat less popular devices with release users.

https://etherpad.mozilla.org/udggXYz1Yg
Comment 5 Maire Reavy [:mreavy] (On PTO until July 5th) 2012-10-11 16:11:56 PDT
(In reply to Chris Peterson (:cpeterson) from comment #4)
> The Galaxy S II (GT-I9100 model) and Galaxy Note (GT-N7000 model) are
> popular devices that shipped with GB (but offer an optional upgrade to ICS).
> The HTC Desire HD and Galaxy S are somewhat less popular devices with
> release users.

Thanks, Chris (Peterson).  So one of those devices (running GB) seems like a good choice -- maybe the Galaxy S II?  How easily/quickly can we ship one of those to Chris Double?  (If you're not the right person to ask about getting and shipping a Galaxy S II to our NZ office, who is?)
Comment 6 Chris Peterson [:cpeterson] 2012-10-11 16:16:32 PDT
The Galaxy S II is available through IT's Service Now portal, but I don't know how they handle international orders.

https://mozilla.service-now.com/
Comment 7 Chris Peterson [:cpeterson] 2012-10-11 16:19:22 PDT
Maire, I just confirmed that Service Now will work internationally. IT will either have a vendor mail the phone directly to the NZ office or, if necessary, ship it from a US office.
Comment 8 cajbir (:cajbir) 2012-10-28 21:42:55 PDT
I have the Samsung Galaxy S II now, thanks. Sadly while the existing patch works on the Otoro GB and Samsung Galaxy Y, it fails to work on the SGS 2. I'm investigating.
Comment 9 cajbir (:cajbir) 2012-10-29 19:59:23 PDT
(In reply to Chris Double (:doublec) from comment #8)
> I have the Samsung Galaxy S II now, thanks. Sadly while the existing patch
> works on the Otoro GB and Samsung Galaxy Y, it fails to work on the SGS 2.
> I'm investigating.

I've worked out why the video wasn'tloading on the SGS 2. The DataSource class seems to use off_t for readAt and getSize on the SGS 2 running GB. On the Otoro and Galaxy Y running GB it uses off64_t.

The SGS 2 is running GB 2.3.4. The Galaxy Y and Otoro is 2.3.6.
Comment 10 Maire Reavy [:mreavy] (On PTO until July 5th) 2012-11-07 18:39:09 PST
blassey, cpeterson (or anyone who as the tools to help):

1) Is it possible for someone to load version 2.3.6 onto a Samsung Galaxy S2 (SGS2)?  If so, can someone then load the patch from this bug onto the phone and see if it plays video and audio?

2) Could we try the patch from this bug on a Samsung Galaxy Note?  (See Comment 4 above -- the Note was the other device we said we could choose as our first device.)  If we try this on the Note, can we check what version of GB software it is running (2.3.4, 2.3.6, something else)?  If possible, could we could try applying the patch on a Note running version 2.3.6 first and then on a Note running 2.3.4?  That would be helpful to test.  If the note won't run 2.3.4 or 2.3.6, that's useful to know.

The purpose of trying the Note (different hardware) is to see if the SGS2 is an outlier.  Thanks!
Comment 11 Chris Peterson [:cpeterson] 2012-11-08 13:18:17 PST
blassey, do you want to take a look at this?

I have a Galaxy S2 running Android 2.3.4 and a Galaxy Note running Android 2.3.5.

I installed doublec's patch (with some tweaks to compile). My Galaxy S2 and Note display the video's playback controls, but pressing play just displays a white box and no audio.

I have not investigated any further because I'm working on B2G bugs.
Comment 12 cajbir (:cajbir) 2012-11-08 16:20:39 PST
(In reply to Chris Peterson (:cpeterson) from comment #11)

> I installed doublec's patch (with some tweaks to compile). My Galaxy S2 and
> Note display the video's playback controls, but pressing play just displays
> a white box and no audio.

That sounds like what I'm seeing on the S2. Thanks for confirming it's on 2.3.5 for the note too.
Comment 13 cajbir (:cajbir) 2012-11-14 13:59:37 PST
What's a non-Samsung phone running GB that we want to support - and can I obtain one for testing please.
Comment 14 Maire Reavy [:mreavy] (On PTO until July 5th) 2012-11-18 20:03:27 PST
(In reply to Chris Double (:doublec) from comment #13)
> What's a non-Samsung phone running GB that we want to support - and can I
> obtain one for testing please.

blassey, cpeterson -- How about The HTC Desire HD?  cpeterson mentioned that as a possibility in Comment 4 above. 

blassey -- https://mozilla.service-now.com/ lists "HTC Desire HD" as available, and they are able to ship to NZ in 2 days time.  The trick is getting the order expedited from the very beginning.  Do you have the ability/authorization to expedite orders through service-now?
Comment 15 Chris Peterson [:cpeterson] 2012-11-19 14:51:25 PST
(In reply to Maire Reavy [:mreavy] from comment #14)
> (In reply to Chris Double (:doublec) from comment #13)
> > What's a non-Samsung phone running GB that we want to support - and can I
> > obtain one for testing please.
> 
> blassey, cpeterson -- How about The HTC Desire HD?  cpeterson mentioned that
> as a possibility in Comment 4 above. 

The HTC Desire HD should work, though it is about 2 years old. It has GB.
Comment 16 Maire Reavy [:mreavy] (On PTO until July 5th) 2012-11-21 18:57:23 PST
I'm working with pdang (Patrick Dang) to ship an HTC Desire HD in MV to doublec in NZ. I have asked that this phone ship this week and to be expedited (2-day).
Comment 17 cajbir (:cajbir) 2012-11-25 19:41:20 PST
Created attachment 685016 [details] [diff] [review]
Part 1: content/media/plugin changes

Changes the way the omx plugin library is detected and loaded so different versions can be loaded based on Android version. Fixes a playback bug in MediaPluginReader for when zero length frames are returned.
Comment 18 cajbir (:cajbir) 2012-11-25 20:07:03 PST
Created attachment 685020 [details] [diff] [review]
Part1: Media plugin backend changes for Gingerbread support

Adds commit header to previous patch
Comment 19 cajbir (:cajbir) 2012-11-25 20:07:58 PST
Created attachment 685021 [details] [diff] [review]
Part2: Add libomxplugin support for Gingerbread

Adds support to media/omx-plugin for Gingerbread
Comment 20 cajbir (:cajbir) 2012-11-25 20:08:59 PST
Created attachment 685022 [details] [diff] [review]
Part3: Add Android OS headers for Gingerbread

Android OS headers for Gingerbread
Comment 21 cajbir (:cajbir) 2012-11-25 20:09:43 PST
Created attachment 685023 [details] [diff] [review]
Part4: Add libstagefright stub libraries for Gingerbread

Stub libraries for libstagefright on Gingerbread
Comment 22 cajbir (:cajbir) 2012-11-25 20:11:41 PST
Created attachment 685024 [details] [diff] [review]
Part5: Package gingerbread libomxplugin libraries on Android

Package Gingerbread omx plugin libraries on android
Comment 23 cajbir (:cajbir) 2012-11-25 20:14:49 PST
Created attachment 685025 [details] [diff] [review]
Part6: Build changes for gingerbread libomxplugin libraries on Android

Toolkit changes for new libomxplugins for gingerbread
Comment 24 cajbir (:cajbir) 2012-11-25 20:16:35 PST
Created attachment 685026 [details] [diff] [review]
Part7: Add Android OS Release version to nsSystemInfo

Adds the Android OS Release version number to nsSystemInfo so we can use this to decide what libomxplugin shared library to load.
Comment 25 Doug Turner (:dougt) 2012-11-26 10:59:41 PST
Comment on attachment 685026 [details] [diff] [review]
Part7: Add Android OS Release version to nsSystemInfo

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

lgtm
Comment 26 Chris Peterson [:cpeterson] 2012-11-26 12:20:58 PST
Comment on attachment 685020 [details] [diff] [review]
Part1: Media plugin backend changes for Gingerbread support

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

Looks good, but I think the Gingerbread and Froyo version checks are incomplete.

::: content/media/plugins/MediaPluginHost.cpp
@@ +132,5 @@
> +// depending on libstagefright version installed on the device and whether it is B2G vs Android.
> +// nullptr is returned if Omx decoding is not supported on the device,
> +static const char* GetOmxLibraryName()
> +{
> +  if (!IsOmxSupported()) 

Remove trailing whitespace. :)

@@ +147,1 @@
>    }

Should we return nullptr if version < 9 (i.e. Froyo or older)? Or can we rely on the other blacklist code to block <= Froyo?

@@ +151,5 @@
> +  if (NS_SUCCEEDED(rv)) {
> +    ALOG("Android Release Version is: %s", NS_LossyConvertUTF16toASCII(release_version).get());
> +  }
> +
> +  if (version == 10 && release_version <= NS_LITERAL_STRING("2.3.5")) {

Gingerbread versions 2.3.0, 2.3.1, and 2.3.2 have API level 9. If we want to use libomxplugingb235.so on those versions of Gingerbread, we will need to check version == 9 or version == 10.

@@ +156,5 @@
> +    // Gingerbread versions from 2.3.5 and below have a different DataSource
> +    // than 2.3.6 and above.
> +    return "lib/libomxplugingb235.so";
> +  }
> +  else if (version == 10 && release_version >= NS_LITERAL_STRING("2.3.6")) {

nsString's operator<= does lexical string comparisons. These checks would fail in the (extremely unlikely) event that Google or some OEM releases at Gingerbread 2.3.10. Fortunately, we can probably assume this will not happen so converting the string to a number is probably overkill. :)
Comment 27 Chris Peterson [:cpeterson] 2012-11-26 12:35:26 PST
Comment on attachment 685021 [details] [diff] [review]
Part2: Add libomxplugin support for Gingerbread

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

Looking good, but I have some questions.

::: media/omx-plugin/Makefile.in
@@ +39,5 @@
> +#        lib/gb/libutils \
> +#        lib/gb/libstagefright \
> +#	gb \
> +#        $(NULL)
> +#endif

Is there a reason to keep these code around? I think we should just delete it. That's why we have version control. :)

@@ +45,5 @@
>  CPPSRCS = \
>      OmxPlugin.cpp \
>      $(NULL)
>  
> +#DEFINES += -DMOZ_ANDROID_GB

Delete commented out code?

::: media/omx-plugin/OmxPlugin.cpp
@@ +102,5 @@
>  namespace OmxPlugin {
>  
>  const int OMX_QCOM_COLOR_FormatYVU420PackedSemiPlanar32m4ka = 0x7FA30C01;
>  const int OMX_QCOM_COLOR_FormatYVU420SemiPlanar = 0x7FA30C00;
> +#if defined(MOZ_ANDROID_GB)

Do we really need to protect this OMX_TI_COLOR definition with #ifdef MOZ_ANDROID_GB?

@@ +257,5 @@
>    int32_t flags = 0;
>    aPluginHost->GetIntPref("media.stagefright.omxcodec.flags", &flags);
>    if (flags != 0) {
>      LOG("media.stagefright.omxcodec.flags=%d", flags);
> +#if !defined(MOZ_ANDROID_GB)

Why don't we want these LOG messages for !MOZ_ANDROID_GB builds?

@@ +317,1 @@
>      flags = OMXCodec::kSoftwareCodecsOnly;

Why does MOZ_ANDROID_GB want flags 0 instead of kSoftwareCodecsOnly?

@@ +515,2 @@
>    if (!format->findRect(kKeyCropRect, &mVideoCropLeft, &mVideoCropTop,
>                                        &cropRight, &cropBottom)) {

Does Gingerbread return no or invalid crop rect values? Rather than duplicating this crop rect code below, I think we should either let GB call (and fail) findRect() or, if necessary, only #ifdef out the findRect() check (so GB skips the check and always fall through to the "assuming no cropping" block).

::: media/omx-plugin/gb/OmxPlugin_2_3_6.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Why does OmxPlugin_2_3_6.cpp's filename have underscores, but OmxPlugin235.cpp and gb235 directory do not? I think the file should just be called OmxPlugin236.cpp.
Comment 28 Chris Peterson [:cpeterson] 2012-11-26 12:38:25 PST
Comment on attachment 685022 [details] [diff] [review]
Part3: Add Android OS headers for Gingerbread

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

LGTM
Comment 29 Chris Peterson [:cpeterson] 2012-11-26 12:45:16 PST
Comment on attachment 685021 [details] [diff] [review]
Part2: Add libomxplugin support for Gingerbread

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

::: media/omx-plugin/gb/OmxPlugin_2_3_6.cpp
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#define MOZ_STAGEFRIGHT_OFF_T off64_t
> +#define MOZ_ANDROID_GB
> +#include "../OmxPlugin.cpp"

Do OmxPlugin235.cpp and OmxPlugin236.cpp need #define MOZ_ANDROID_GB? This macro is #defined in their Makefiles. I think we should #define the macro either only in these .cpp files (my slight preference) or only in the Makefiles.
Comment 30 Chris Peterson [:cpeterson] 2012-11-26 12:46:55 PST
Comment on attachment 685023 [details] [diff] [review]
Part4: Add libstagefright stub libraries for Gingerbread

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

LGTM, but consider my comment on the other patch about #defining MOZ_ANDROID_GB in either the .cpp files or Makefiles.
Comment 31 cajbir (:cajbir) 2012-11-26 20:05:04 PST
Created attachment 685467 [details] [diff] [review]
Part1: Media plugin backend changes for Gingerbread support

Addresses review comments.
Comment 32 cajbir (:cajbir) 2012-11-26 20:08:26 PST
Created attachment 685470 [details] [diff] [review]
Part2: Add libomxplugin support for Gingerbread

Addresses review comments. 

> Why does MOZ_ANDROID_GB want flags 0 instead of kSoftwareCodecsOnly?

kSoftwareCodecsOnly doesn't exist in Gingerbread. It does have one for preferring software codecs over hardware so I changed the 0 to use that.

> Does Gingerbread return no or invalid crop rect values? Rather
> than duplicating this crop rect code below, I think we should
> either let GB call (and fail) findRect() or, if necessary,
> only #ifdef out the findRect() check (so GB skips the check
> and always fall through to the "assuming no cropping" block).

Gingerbread doesn't have the key value kKeyCropRect. I've changed to the last option you suggested.
Comment 33 Chris Peterson [:cpeterson] 2012-11-27 10:18:38 PST
Comment on attachment 685467 [details] [diff] [review]
Part1: Media plugin backend changes for Gingerbread support

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

LGTM
Comment 34 Chris Peterson [:cpeterson] 2012-11-27 10:19:13 PST
Comment on attachment 685470 [details] [diff] [review]
Part2: Add libomxplugin support for Gingerbread

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

LGTM
Comment 35 cajbir (:cajbir) 2012-11-29 18:45:53 PST
Push to try to ensure I haven't broken other platform builds:
https://tbpl.mozilla.org/?tree=Try&rev=e708ff3bd5a8

Will land if that's clear.
Comment 38 alex_mayorga 2012-12-03 13:41:13 PST
How can I verify this bug on a Sony Ericsson MK16a with Android 2.3.4?
Comment 39 cajbir (:cajbir) 2012-12-03 14:39:49 PST
(In reply to alex_mayorga from comment #38)
> How can I verify this bug on a Sony Ericsson MK16a with Android 2.3.4?

In about:config, set "stagefright.force-enabled" to true. Then go to http://cd.pn/b2 and click play. The audio should play. You may or may not get video depending on if the color formats on that device are supported.

I haven't tested on that device so hopefully it works.
Comment 40 Ralph Giles (:rillian) needinfo me 2012-12-03 15:39:24 PST
(In reply to Chris Double (:doublec) from comment #39)

> In about:config, set "stagefright.force-enabled" to true. Then go to
> http://cd.pn/b2 and click play. The audio should play. You may or may not
> get video depending on if the color formats on that device are supported.

On my HTC Nexus One (CM7.1.0-N1) I still get 'Video format or MIME type is not supported' on this page.
Comment 41 cajbir (:cajbir) 2012-12-03 15:46:34 PST
(In reply to Ralph Giles (:rillian) from comment #40)
> On my HTC Nexus One (CM7.1.0-N1) I still get 'Video format or MIME type is
> not supported' on this page.

Please raise a separate bug for it and attach an "adb logcat" around the time of loading the video. I'm interested in any logcat line containing "Omx", "OMX", or "MediaPluginHost". Unfortunately most devices have variants of stagefright and it's a matter of finding workaround for the variants.
Comment 42 Ralph Giles (:rillian) needinfo me 2012-12-03 16:15:22 PST
See bug 817868 for more on the stagefright support on my Android 2.3 device.
Comment 43 alex_mayorga 2012-12-04 20:33:28 PST
Mr. Double,

Thanks! I tried as suggested but Nightly ends up crashing.
I've filed the crashes as https://bugzilla.mozilla.org/show_bug.cgi?id=818363 FWIW.

Note You need to log in before you can comment on or make changes to this bug.