Closed Bug 787228 Opened 12 years ago Closed 12 years ago

Enable use of hardware H.264/AAC/MP3 decoders in Android libstagefright omx plugin for GB

Categories

(Core :: Audio/Video, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: mreavy, Assigned: cajbir)

References

Details

Attachments

(7 files, 5 obsolete files)

481.27 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
11.20 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
1.01 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
3.19 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.49 KB, patch
dougt
: review+
Details | Diff | Splinter Review
6.24 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
11.52 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
Bug 782508 added support for the hardware decoders accessable via libstagefright for Android ICS/JB.  This adds hardware decoding support for GB (Gingerbread).
OS: Linux → Android
Hardware: x86 → ARM
Blocks: 755364
Attached patch work in progress patch (obsolete) — Splinter Review
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.
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
Attached patch work in progress patch (obsolete) — Splinter Review
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.
Attachment #670203 - Attachment is obsolete: true
Depends on: 787229
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.
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
(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?)
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/
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.
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.
(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.
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!
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.
(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.
What's a non-Samsung phone running GB that we want to support - and can I obtain one for testing please.
(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?
(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.
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).
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.
Attachment #670278 - Attachment is obsolete: true
Attachment #685016 - Flags: review?(cpeterson)
Adds commit header to previous patch
Attachment #685016 - Attachment is obsolete: true
Attachment #685016 - Flags: review?(cpeterson)
Attachment #685020 - Flags: review?(cpeterson)
Adds support to media/omx-plugin for Gingerbread
Attachment #685021 - Flags: review?(cpeterson)
Android OS headers for Gingerbread
Attachment #685022 - Flags: review?(cpeterson)
Stub libraries for libstagefright on Gingerbread
Attachment #685023 - Flags: review?(cpeterson)
Package Gingerbread omx plugin libraries on android
Attachment #685024 - Flags: review?(mark.finkle)
Toolkit changes for new libomxplugins for gingerbread
Attachment #685025 - Flags: review?(khuey)
Adds the Android OS Release version number to nsSystemInfo so we can use this to decide what libomxplugin shared library to load.
Attachment #685026 - Flags: review?(doug.turner)
Comment on attachment 685026 [details] [diff] [review]
Part7: Add Android OS Release version to nsSystemInfo

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

lgtm
Attachment #685026 - Flags: review?(doug.turner) → review+
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. :)
Attachment #685020 - Flags: review?(cpeterson) → review-
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.
Attachment #685021 - Flags: review?(cpeterson) → review-
Comment on attachment 685022 [details] [diff] [review]
Part3: Add Android OS headers for Gingerbread

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

LGTM
Attachment #685022 - Flags: review?(cpeterson) → review+
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 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.
Attachment #685023 - Flags: review?(cpeterson) → review+
Attachment #685024 - Flags: review?(mark.finkle) → review+
Addresses review comments.
Attachment #685020 - Attachment is obsolete: true
Attachment #685467 - Flags: review?(cpeterson)
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.
Attachment #685021 - Attachment is obsolete: true
Attachment #685470 - Flags: review?(cpeterson)
Blocks: 805700
Comment on attachment 685467 [details] [diff] [review]
Part1: Media plugin backend changes for Gingerbread support

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

LGTM
Attachment #685467 - Flags: review?(cpeterson) → review+
Comment on attachment 685470 [details] [diff] [review]
Part2: Add libomxplugin support for Gingerbread

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

LGTM
Attachment #685470 - Flags: review?(cpeterson) → review+
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.
How can I verify this bug on a Sony Ericsson MK16a with Android 2.3.4?
(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.
(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.
(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.
Blocks: 817868
See bug 817868 for more on the stagefright support on my Android 2.3 device.
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.
Blocks: 820236
Blocks: 823253
Depends on: 829454
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: