Last Comment Bug 782508 - Enable use of hardware H.264/AAC/MP3 decoders in Android libstagefright omx plugin for ICS/JB
: 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)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla17
Assigned To: cajbir (:cajbir)
:
: Maire Reavy [:mreavy]
Mentors:
Depends on: 787319 804768 759945 785340 803794 817478 853522
Blocks: 748351 755364 787227
  Show dependency treegraph
 
Reported: 2012-08-13 21:02 PDT by cajbir (:cajbir)
Modified: 2013-04-30 00:22 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Enable use of hardware decoders (5.69 KB, patch)
2012-08-13 21:05 PDT, cajbir (:cajbir)
no flags Details | Diff | Splinter Review
Enable use of hardware decoders (7.66 KB, patch)
2012-08-13 22:17 PDT, cajbir (:cajbir)
no flags Details | Diff | Splinter Review
Enable use of hardware decoders (21.02 KB, patch)
2012-08-20 17:32 PDT, cajbir (:cajbir)
cpeterson: review+
Details | Diff | Splinter Review

Description cajbir (:cajbir) 2012-08-13 21:02:25 PDT
Bug 759945 added support for software decoding for H.264/AAC/MP3 using libstagefright. This bug is for add support for the hardware decoders accessable via libstagefright.
Comment 1 cajbir (:cajbir) 2012-08-13 21:05:58 PDT
Created attachment 651624 [details] [diff] [review]
Enable use of hardware decoders

Quick change based on information from Chris Peterson to enable use of the hardware decoders. With this patch applied it successfully instantiates and uses the hardware decoders on a Nexus S.
Comment 2 cajbir (:cajbir) 2012-08-13 22:17:34 PDT
Created attachment 651631 [details] [diff] [review]
Enable use of hardware decoders

Tidied up a bit. I'll test I haven't broken B2G then request review.
Comment 3 Chris Peterson [:cpeterson] 2012-08-14 07:18:21 PDT
Comment on attachment 651631 [details] [diff] [review]
Enable use of hardware decoders

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

::: media/omx-plugin/OmxPlugin.cpp
@@ +70,5 @@
>  
>  ssize_t MediaStreamSource::readAt(off64_t offset, void *data, size_t size)
>  {
>    char *ptr = reinterpret_cast<char *>(data);
> + size_t todo = size;

You inadvertently changed the indentation.

@@ +101,5 @@
>  
>  class OmxDecoder {
>    PluginHost *mPluginHost;
>    Decoder *mDecoder;
> +  OMXClient *mClient;

You don't need to allocate the OMXClient with `new`. You can just declare `OMXClient mClient` as a member object.

@@ +266,5 @@
>    int64_t totalDurationUs = 0;
>  
> +  mClient = new OMXClient;
> +  if (mClient->connect() != OK) {
> +    LOG("OMXClient failed to connect");

You might want to add a comment that the current implementation of OMXClient::connect() always returns status_t OK. It will abort() if it fails to connect! If we actually hit that case, we might need to reimplement OMXClient::connect() ourselves without Stagefright's fatal CHECK() macros.
Comment 4 cajbir (:cajbir) 2012-08-19 21:55:41 PDT
This does indeed break B2G video playback sadly. I'm investigating.
Comment 5 Chris Peterson [:cpeterson] 2012-08-20 12:42:10 PDT
(In reply to Chris Double (:doublec) from comment #4)
> This does indeed break B2G video playback sadly. I'm investigating.

Does B2G use a separate mediaserver daemon? Maybe we need something like a "CodecConnection" class to encapsulate from OmxPlugin whether we are:

1. using a software decoder in the same process (No OMXClient->connect() needed)
2. using a hardware decoder in a remote mediaserver process (need OMXClient->connect())
3. or B2G using hardware decoder in the same process?
Comment 6 cajbir (:cajbir) 2012-08-20 16:51:36 PDT
Given that the plugin code eventually won't be used on b2g when bug 759506 lands I'm just #ifdef'ing around the B2G vs Android code.
Comment 7 cajbir (:cajbir) 2012-08-20 17:32:10 PDT
Created attachment 653592 [details] [diff] [review]
Enable use of hardware decoders
Comment 8 Chris Peterson [:cpeterson] 2012-08-20 18:45:53 PDT
Comment on attachment 653592 [details] [diff] [review]
Enable use of hardware decoders

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

LGTM with nit.

cdouble, do you plan to uplift this change to Aurora 16 or will 16 only support software decoding?

::: media/omx-plugin/OmxPlugin.cpp
@@ +286,5 @@
> +  if (mClient.connect() != OK) {
> +    LOG("OMXClient failed to connect");
> +  }
> +  sp<IOMX> omx = mClient.interface();
> +  uint32_t flags = 0;

Maybe add a comment describing what OMXCodec will do when `flags = 0` (i.e. return either a software or hardware decoder).

btw, I anticipate we will need to add our own content sniffing so we can specifically ask OMXCodec for kHardwareCodecsOnly or kSoftwareCodecsOnly. AFAICT, OMXCodec simply matches codecs by MIME type without checking whether the hardware supports a given H.264 Profile, Level, or dimension.
Comment 9 cajbir (:cajbir) 2012-08-20 19:33:17 PDT
(In reply to Chris Peterson (:cpeterson) from comment #8)
> cdouble, do you plan to uplift this change to Aurora 16 or will 16 only
> support software decoding?

I'll leave this up to the Android team to decide.

> 
> Maybe add a comment describing what OMXCodec will do when `flags = 0` (i.e.
> return either a software or hardware decoder).

I'll do this before committing, thanks.

> btw, I anticipate we will need to add our own content sniffing so we can
> specifically ask OMXCodec for kHardwareCodecsOnly or kSoftwareCodecsOnly.
> AFAICT, OMXCodec simply matches codecs by MIME type without checking whether
> the hardware supports a given H.264 Profile, Level, or dimension.

Yes, or possibly try hardware first, fallback to software if it fails. A basic content sniffer was recently added to handle application/octet stream sniffing IIRC. Possibly this could be extended.
Comment 11 Ed Morley [:emorley] 2012-08-21 06:27:56 PDT
https://hg.mozilla.org/mozilla-central/rev/ba7652be9d09
Comment 12 Maire Reavy [:mreavy] 2012-08-30 14:26:45 PDT
Changing the title to make it clear this solution is just for ICS/JB.  I'm filing a new bug for GB support.
Comment 13 Maire Reavy [:mreavy] 2012-08-30 15:28:18 PDT
Moving dependency (Bug 785275) to Bug 787227.  Leaving Bug 785340 as a dependency because that looks like a true dependency (not hardware specific bug).

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