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

RESOLVED FIXED in mozilla17

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

(Depends on: 2 bugs, Blocks: 3 bugs)

Trunk
mozilla17
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 759945
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
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.
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.
(Assignee)

Comment 4

5 years ago
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?
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Created attachment 653592 [details] [diff] [review]
Enable use of hardware decoders
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+
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba7652be9d09
https://hg.mozilla.org/mozilla-central/rev/ba7652be9d09
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
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

Updated

5 years ago
Depends on: 787319

Updated

5 years ago
Depends on: 803794

Updated

5 years ago
Depends on: 804768
(Assignee)

Updated

5 years ago
Depends on: 817478

Updated

4 years ago
Depends on: 853522
You need to log in before you can comment on or make changes to this bug.