Closed Bug 782508 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Audio/Video, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: cajbir, Assigned: cajbir)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Depends on: 759945
Attached patch Enable use of hardware decoders (obsolete) — Splinter Review
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.
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
Attached patch Enable use of hardware decoders (obsolete) — Splinter Review
Tidied up a bit. I'll test I haven't broken B2G then request review.
Attachment #651624 - Attachment is obsolete: true
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.
This does indeed break B2G video playback sadly. I'm investigating.
(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?
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.
Attachment #651631 - Attachment is obsolete: true
Attachment #653592 - Flags: review?(cpeterson)
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.
Attachment #653592 - Flags: review?(cpeterson) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/ba7652be9d09
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 785340
Depends on: 785275
Changing the title to make it clear this solution is just for ICS/JB.  I'm filing a new bug for GB support.
Summary: Enable use of hardware H.264/AAC/MP3 decoders in Android libstagefright omx plugin → Enable use of hardware H.264/AAC/MP3 decoders in Android libstagefright omx plugin for ICS/JB
Blocks: 787227
Moving dependency (Bug 785275) to Bug 787227.  Leaving Bug 785340 as a dependency because that looks like a true dependency (not hardware specific bug).
Blocks: 748351, 755364
No longer depends on: 785275
Depends on: 787319
Depends on: 803794
Depends on: 804768
Depends on: 817478
Depends on: 853522
You need to log in before you can comment on or make changes to this bug.