Closed Bug 821160 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Audio/Video, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: cajbir, Assigned: cajbir)

References

Details

Attachments

(6 files, 2 obsolete files)

Add FroYo (Android 2.2.x) support for libstagefright decoding.
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
Attachment #691673 - Flags: review?(cpeterson)
Attachment #691674 - Flags: review?(cpeterson)
Attachment #691675 - Flags: review?(cpeterson)
Attachment #691676 - Flags: review?(cpeterson)
Attachment #691677 - Flags: review?(mark.finkle)
These six patches add hardware and software decoding support for Froyo android devices. Tested on a LG-P970 running Android 2.2.2.
Comment on attachment 691677 [details] [diff] [review]
Part5: Package froyo libomxplugin libraries on Android

Fits the pattern :)
Attachment #691677 - Flags: review?(mark.finkle) → review+
Comment on attachment 691673 [details] [diff] [review]
Part1: Media plugin backend changes for Froyo support

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

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

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

LGTM with nits.

::: media/omx-plugin/OmxPlugin.cpp
@@ +401,5 @@
>    if (videoTrackIndex != -1 && (videoTrack = extractor->getTrack(videoTrackIndex)) != NULL) {
> +#if defined(MOZ_ANDROID_FROYO)
> +    sp<MetaData> meta = extractor->getTrackMetaData(videoTrackIndex);
> + 
> +    int32_t maxInputSize;

I think you should just delete the previous two lines. The `maxInputSize` variable is unused and the line before that has trailing whitespace. :)

@@ +564,1 @@
>      LOG("rotation not available, assuming 0");

Instead of adding all these #ifdefs, can we just allow findInt32(kKeyRotation) to "fail" on Froyo? The "error" path codes the right thing and the LOG() message is accurate.
Attachment #691674 - Flags: review?(cpeterson) → review+
Comment on attachment 691675 [details] [diff] [review]
Part3: Add Android OS headers for Froyo

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

LGTM
Attachment #691675 - Flags: review?(cpeterson) → review+
Comment on attachment 691676 [details] [diff] [review]
Part4: Add libstagefright stub libraries for Froyo

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

Looking good, but I have a question about MOZ_ANDROID_GB:

::: media/omx-plugin/lib/froyo/libstagefright/libstagefright.cpp
@@ +5,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#define MOZ_STAGEFRIGHT_OFF_T off_t
> +#define MOZ_ANDROID_GB
> +#define MOZ_ANDROID_FROYO
> +#include "../../gb/libstagefright/libstagefright.cpp"

Why are we #defining MOZ_ANDROID_GB for Froyo? I can understand #defining MOZ_ANDROID_FROYO in GB files to indicate backwards compatible support for Froyo features on GB, but #defining MOZ_ANDROID_GB on Froyo seems strange. If the MOZ_ANDROID_GB #ifdefs in OmxPlugin.cpp are applicable for Froyo, maybe we should change those #ifdef names?
Attachment #691676 - Flags: review?(cpeterson) → review-
(In reply to Chris Peterson (:cpeterson) from comment #10)
> Instead of adding all these #ifdefs, can we just allow
> findInt32(kKeyRotation) to "fail" on Froyo? The "error" path codes the right
> thing and the LOG() message is accurate.

Unfortunately kKeyRotation is not defined in the froyo header files so the line doesn't compile rather than just fail.
Addressed review comment and changed the MOZ_ANDROID_GB/FROYO so that a MOZ_ANDROID_V2_X_X is defined if either is set, and checks are made on that instead, as part of addressing your part 4 review comment. I'm re-requesting review to check that's ok.
Attachment #691674 - Attachment is obsolete: true
Attachment #692055 - Flags: review?(cpeterson)
Remove MOZ_ANDROID_GB as per review comment.
Attachment #691676 - Attachment is obsolete: true
Attachment #692056 - Flags: review?(cpeterson)
Attachment #692056 - Flags: review?(cpeterson)
Attachment #692056 - Attachment description: Part6: Build changes for froyo libomxplugin libraries on Android → Part 4: Add libstagefright stub libraries for Froyo - r=cpeterson
Attachment #692056 - Flags: review?(cpeterson)
Comment on attachment 692055 [details] [diff] [review]
Part2: Add libomxplugin support for froyo

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

LGTM!
Attachment #692055 - Flags: review?(cpeterson) → review+
Comment on attachment 692056 [details] [diff] [review]
Part 4: Add libstagefright stub libraries for Froyo - r=cpeterson

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

LGTM
Attachment #692056 - Flags: review?(cpeterson) → review+
Bustage fix for Froyo debug builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d433d1a9fd78
Blocks: 823253
This patch resulted in some junk files:

 media/omx-plugin/include/froyo/stagefright/DataSource.h.orig
 media/omx-plugin/include/froyo/stagefright/DataSource.h.rej

I've removed them:

  https://hg.mozilla.org/mozilla-central/rev/6e61c4a26591
  https://hg.mozilla.org/integration/mozilla-inbound/rev/79d9e2139227

Consider adding 
  *.rej
  *.orig
  *.pyc

to your hgignore file :)
Can we add a mercurial hook to reject files ending with *.rej, *.orig, *.pyc, *~, or whatever?
You need to log in before you can comment on or make changes to this bug.