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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

Add FroYo (Android 2.2.x) support for libstagefright decoding.

Comment 1

[cajbir (:cajbir)]

Attachment: Part1: Media plugin backend changes for Froyo support

Comment 2

[cajbir (:cajbir)]

Attachment: Part2: Add libomxplugin support for froyo

Comment 3

[cajbir (:cajbir)]

Attachment: Part3: Add Android OS headers for Froyo

Comment 4

[cajbir (:cajbir)]

Attachment: Part4: Add libstagefright stub libraries for Froyo

Comment 5

[cajbir (:cajbir)]

Attachment: Part5: Package froyo libomxplugin libraries on Android

Comment 6

[cajbir (:cajbir)]

Attachment: Part6: Build changes for froyo libomxplugin libraries on Android

Comment 7

[cajbir (:cajbir)]

These six patches add hardware and software decoding support for Froyo android devices. Tested on a LG-P970 running Android 2.2.2.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 691677 [details] [diff] [review]
Part5: Package froyo libomxplugin libraries on Android

Fits the pattern :)

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 691673 [details] [diff] [review]
Part1: Media plugin backend changes for Froyo support

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

LGTM

Comment 10

[Chris Peterson [:cpeterson]]

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.

Comment 11

[Chris Peterson [:cpeterson]]

Comment on attachment 691675 [details] [diff] [review]
Part3: Add Android OS headers for Froyo

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

LGTM

Comment 12

[Chris Peterson [:cpeterson]]

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?

Comment 13

[cajbir (:cajbir)]

(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.

Comment 14

[cajbir (:cajbir)]

Attachment: Part2: Add libomxplugin support for froyo

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.

Comment 15

[cajbir (:cajbir)]

Attachment: Part 4: Add libstagefright stub libraries for Froyo - r=cpeterson

Remove MOZ_ANDROID_GB as per review comment.

Comment 16

[Chris Peterson [:cpeterson]]

Comment on attachment 692055 [details] [diff] [review]
Part2: Add libomxplugin support for froyo

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

LGTM!

Comment 17

[Chris Peterson [:cpeterson]]

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

Comment 18

[cajbir (:cajbir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf4ae2968bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/49ff58ad7d24
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff7877b3133
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2058a476b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a641f55054a
https://hg.mozilla.org/integration/mozilla-inbound/rev/143ce885103a

Comment 19

[cajbir (:cajbir)]

Bustage fix for Froyo debug builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d433d1a9fd78

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/aaf4ae2968bb
https://hg.mozilla.org/mozilla-central/rev/49ff58ad7d24
https://hg.mozilla.org/mozilla-central/rev/9ff7877b3133
https://hg.mozilla.org/mozilla-central/rev/4a2058a476b5
https://hg.mozilla.org/mozilla-central/rev/8a641f55054a
https://hg.mozilla.org/mozilla-central/rev/143ce885103a
https://hg.mozilla.org/mozilla-central/rev/d433d1a9fd78

Comment 21

[Jason Duell]

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 :)

Comment 22

[Masatoshi Kimura [:emk]]

Can we add a mercurial hook to reject files ending with *.rej, *.orig, *.pyc, *~, or whatever?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/79d9e2139227