1026305 - Use gralloc on Android for rendering hardware accelerated OMX video frames

Status: RESOLVED WONTFIX

(Core :: Audio/Video: Playback, defect)

Comment 1

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Fat fingered that one.

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Use gralloc for hw accelerated OMX buffers

This patch isn't quite ready for review yet, but hoping to get feedback on the approach.

This patch:
 - Enables gralloc-related parts of the layers system on Android (previously only used on b2g).

Using gralloc means that we can get the hardware to do colour conversion for us where it's available. This should lead to gains in both performance and reduced color converter-related crashiness.

 - Depends on far fewer ABI assumptions than before.

We depend only on method signatures, the Android refcount implementation, and the layout of I420ColorConverter. Almost all functions we use are dlopen'd instead of linked against, so there is less risk of weird segfaults from trying to call NULL functions.

 - Moves colour conversion out of OMX plugin and into an OMXImageHelper, which takes a bunch of data from the OMX decoder and returns a GrallocImage, YCbCrImage, or SharedRGBImage.

A lot of colour conversion code is duplicated between Android OMX, B2G, and soon probably Android MediaCodec. Moving this code out of the plugin means that we can use gecko code during conversion, and share the colour conversion code with other backends.

 - Attempts to automatically map the returned OMX colour format to its corresponding HAL colour format.

There are two colour format types we must deal with: OMX and HAL colour formats, and the mapping between these formats is not always known. The way we handle this is to decode a single known frame with the OMX decoder and try to convert it back to RGB using various HAL formats. This is done once, and the result stored in a pref.

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 970847-2.patch

Previous patch applies on top of this one.

This patch makes the OMX plugin catch up if it's falling behind. It also improves bitrate estimation for media plugins: we usually estimate this from the current stream position, but the MediaResourceServer consumes data faster than the codec, which can overestimate the current bitrate; so now it uses a stream position estimated from currentTime / mediaDuration instead.

Comment 4

[cajbir (:cajbir)]

Comment on attachment 8452815 [details] [diff] [review]
Use gralloc for hw accelerated OMX buffers

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

content/media stuff looks fine to me. The Gralloc stuff is outside my knowledge so I leave that to others. Might need sec review for the dlopen stuff.

::: content/media/plugins/MPAPI.h
@@ +44,5 @@
>    int32_t mSkip;
>  };
>  
>  struct VideoFrame {
> +  DataFormat mDataFormat;

Comment stating that this is the type of the data held in the 'mData' field and the data in the union mMetaData (I assume - I'm guessing). Move it and mData down to before the the union itself so they are lexically close.

DataFormat names are YCBCR and ANDROID_OMX but union members are YCbCr and AndroidExternal. They should match so maybe rename AndroidExternal or ANDROID_OMX. Add comments explaining their relationship.

::: content/media/plugins/MediaPluginDecoder.cpp
@@ +16,5 @@
> +
> +#include "nsIRandomGenerator.h"
> +
> +#include <unistd.h>
> +

Why the random newlines between some of the includes?

@@ +63,5 @@
> +  MPAPI::Decoder *plugin =
> +    GetMediaPluginHost()->CreateDecoder(resource, MP4_MIME);
> +
> +  if (!plugin) {
> +    goto fail;

Given that the fail case is just a return we should just return that here and other places you use goto. You use auto scoped classes here and use of 'goto' makes it harder for the reader to reason when the autoscoped classes destructor activates.

@@ +67,5 @@
> +    goto fail;
> +  }
> +
> +  if (!plugin->ReadVideo(plugin, &frame, -1, -1, nullptr)) {
> +    goto fail;

Do we need to "GetMediaPluginHost()->DestroyDecoder(plugin);" and other places where we fail?

::: content/media/plugins/MediaPluginDecoder.h
@@ +16,5 @@
>    nsCString mType;
>  public:
>    MediaPluginDecoder(const nsACString& aType);
>  
> +  static nsresult DecodeReferenceFrame(uint8_t** aResult,

Comment explaining what this is for.

::: content/media/plugins/MediaPluginReader.cpp
@@ +40,5 @@
>  }
>  
>  nsresult MediaPluginReader::Init(MediaDecoderReader* aCloneDonor)
>  {
> +  // Ensure the random generator service has been started on the main thread.

Assert that we are on the main thread.

@@ +52,5 @@
> +static bool
> +IsColorFormatSupported(uint32_t aColorFormat)
> +{
> +#ifdef MOZ_WIDGET_ANDROID
> +  printf_stderr("Trying color format %#x\n", aColorFormat);

Is printf_stderr the way of doing logging these days?

@@ +84,5 @@
> +      NS_SUCCEEDED(MediaPluginDecoder::DecodeReferenceFrame(frameData.StartAssignment(),
> +                                                            &frameLen, &colorFormat,
> +                                                            frameSize, frameCrop)) &&
> +      OMXImageHelper::FindVideoColorFormat(frameData.get(), frameLen, colorFormat,
> +                                           frameSize, frameCrop);

Some of the arguments to this function could be uninitialized depending on where DecodeReferenceFrame fails. Either initialize them first or check if DecodeReferenceFrame succeeded before using them.

@@ +333,2 @@
>      }
>   

Remove whitespace.

::: content/media/plugins/ReferenceMediaResource.cpp
@@ +10,5 @@
> +#define MP4_MIME NS_LITERAL_CSTRING("video/mp4")
> +
> +namespace mozilla {
> +
> +const char bytes[] = {

This is a very generic name, 'bytes', for something that is in the 'mozilla' namespace scope. I suggest calling it something more unique or make it static.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Use gralloc for hw accelerated OMX buffers

Generally cleaned the patch up a little bit; haven't addressed the above comments yet.

Comment 6

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8453526 [details] [diff] [review]
Use gralloc for hw accelerated OMX buffers

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

gfx/layers staff looks basically good! :nical is offical gfx/layers' responsible person. It seems better to ask him a feedback.

SharedBufferManagerParent/Child and ShadowLayerUtilsGralloc.cpp changed a lot recently. These parts needs to be updated.

::: content/media/plugins/MediaPluginReader.cpp
@@ +332,2 @@
>      }
>   

// nit remove space.

::: gfx/layers/AndroidConstants.h
@@ +6,5 @@
> +#ifndef __AndroidConstants_h__
> +#define __AndroidConstants_h__
> +
> +namespace android {
> +

Can't we use TypedEnum here like the following?
  http://dxr.mozilla.org/mozilla-central/source/gfx/2d/Types.h

::: gfx/layers/GrallocImages.h
@@ +101,5 @@
> +  virtual nsIntRect GetPictureRect() MOZ_OVERRIDE
> +  {
> +    return mPicRect;
> +  }
> +

TextureClient already has a size info. This seems duplicated to it.

@@ +128,4 @@
>    }
>  
>  private:
> +  nsIntRect mPicRect;

TextureClient already has a size info. This seems duplicated to it.

::: gfx/layers/GraphicBufferWrapper.h
@@ +22,5 @@
> +
> +typedef uint32_t status_t;
> +#endif
> +
> +class GraphicBufferWrapper : public mozilla::RefCounted<GraphicBufferWrapper>

mozilla::RefCounted is deprecated in new code. And this class needs to be thread safe. mozilla::RefCounted is not thread safe.
https://groups.google.com/forum/#!topic/mozilla.dev.platform/Z8muIf0Ax6o

::: gfx/layers/GraphicBufferWrapperGonk.cpp
@@ +58,5 @@
> +  return mGraphicBuffer.get();
> +}
> +
> +uint32_t
> +GraphicBufferWrapper::getWidth() const 

// nit remove space.

@@ +64,5 @@
> +  return mGraphicBuffer->getWidth();
> +}
> +
> +uint32_t
> +GraphicBufferWrapper::getHeight() const 

// nit remove space.

@@ +70,5 @@
> +  return mGraphicBuffer->getHeight();
> +}
> +
> +uint32_t
> +GraphicBufferWrapper::getStride() const 

// nit remove space.

@@ +76,5 @@
> +  return mGraphicBuffer->getStride();
> +}
> +
> +uint32_t
> +GraphicBufferWrapper::getUsage() const 

// nit remove space.

@@ +82,5 @@
> +  return mGraphicBuffer->getUsage();
> +}
> +
> +PixelFormat
> +GraphicBufferWrapper::getPixelFormat() const 

// nit remove space.

::: gfx/layers/LayersTypes.h
@@ +138,1 @@
>    // size of mSurface 

// nit remove space.

::: gfx/layers/OMXImageHelper.cpp
@@ +569,5 @@
> + */
> +static bool
> +TryHALColorFormat(uint8_t* aReferenceData, uint32_t aReferenceLen,
> +                  uint32_t aHALColorFormat, const gfx::IntSize& aSize,
> +                  const gfx::IntRect& aCrop)

It seems better if the function name suggests what is going to do.

@@ +650,5 @@
> +
> +static int32_t
> +TryVideoColorFormats(uint8_t* aReferenceData, uint32_t aReferenceLen,
> +                     uint32_t aOMXColorFormat, const gfx::IntSize& aSize,
> +                     const gfx::IntRect& aCrop)

It seems better if the function name suggests what is going to do.

@@ +731,5 @@
> +  static bool sInitSuccess = false;
> +
> +  if (sInitDone) {
> +    return sInitSuccess;
> +  }

Can't we need to protect by using lock like attachment 8448663 [details] [diff] [review] in Bug 904177?

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +195,5 @@
>  
>  //-----------------------------------------------------------------------------
>  // Parent process
>  
> +// XXX Is this still used anywhere?

Good catch. This can be removed.

::: gfx/layers/ipc/SharedBufferManagerParent.h
@@ +96,3 @@
>    Mutex mBuffersMutex;
>  #endif
>    

//nit remove white space

::: media/omx-plugin/OmxPlugin.cpp
@@ +1056,5 @@
>      }
> +
> +    aFrame->SetAndroid(timeUs, keyFrame, (uint8_t*)data, length, mVideoColorFormat,
> +                       mVideoStride, mVideoSliceHeight, mVideoRotation,
> +                       mVideoCropLeft, mVideoCropTop, mVideoCropRight, mVideoCropBottom);

The function name "SetAndroid()" seems not clear. Can it changed to more clear name?

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Use gralloc for hw accelerated OMX buffers, v3

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> gfx/layers staff looks basically good! :nical is offical gfx/layers'
> responsible person. It seems better to ask him a feedback.
Ok. Will r? him.

> ::: gfx/layers/AndroidConstants.h
> @@ +6,5 @@
> > +#ifndef __AndroidConstants_h__
> > +#define __AndroidConstants_h__
> > +
> > +namespace android {
> > +
> 
> Can't we use TypedEnum here like the following?
>   http://dxr.mozilla.org/mozilla-central/source/gfx/2d/Types.h
I think it's better this way. It's intended as more of a compat layer for when we don't have the android headers, so it should be as close to the android definitions as possible.

> @@ +731,5 @@
> > +  static bool sInitSuccess = false;
> > +
> > +  if (sInitDone) {
> > +    return sInitSuccess;
> > +  }
> 
> Can't we need to protect by using lock like attachment 8448663 [details] [diff] [review]
> [diff] [review] in Bug 904177?
True. Moved init to main thread so this isn't a problem.


(In reply to cajbir (:cajbir) from comment #4)
> @@ +84,5 @@
> > +      NS_SUCCEEDED(MediaPluginDecoder::DecodeReferenceFrame(frameData.StartAssignment(),
> > +                                                            &frameLen, &colorFormat,
> > +                                                            frameSize, frameCrop)) &&
> > +      OMXImageHelper::FindVideoColorFormat(frameData.get(), frameLen, colorFormat,
> > +                                           frameSize, frameCrop);
> 
> Some of the arguments to this function could be uninitialized depending on
> where DecodeReferenceFrame fails. Either initialize them first or check if
> DecodeReferenceFrame succeeded before using them.
That's what the |NS_SUCCEEDED(...) &&| is for.

Comment 8

[cajbir (:cajbir)]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #7)
> Created attachment 8455063 [details] [diff] [review]
> > Some of the arguments to this function could be uninitialized depending on
> > where DecodeReferenceFrame fails. Either initialize them first or check if
> > DecodeReferenceFrame succeeded before using them.
> That's what the |NS_SUCCEEDED(...) &&| is for.

Oh I see - it's the indenting that confused me. I didn't realize it was an argument to DecodeReferenceFrame. It looks like it's called after it. Please fix that if you haven't in the latest patch.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Pretty sure this a non-starter on Android. You need to be root to allocate gralloc buffers (except on some devices that don't actually enforce that like, like PowerVR).

We might be able to use the "single buffer" mode for SurfaceTexture, though, on KitKat and higher. http://developer.android.com/reference/android/graphics/SurfaceTexture.html#SurfaceTexture%28int,%20boolean%29

:jgilbert has written a SurfaceTexture-backed TexClient which could be adapted for that in bug 1037147.

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #9)
> Pretty sure this a non-starter on Android. You need to be root to allocate
> gralloc buffers (except on some devices that don't actually enforce that
> like, like PowerVR).

I have a pile of unrooted Android devices on my desk from ICS onwards powered by QC, TI, nVidia SoCs (Samsung notably missing, but hey) that beg to differ...

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #10)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #9)
> > Pretty sure this a non-starter on Android. You need to be root to allocate
> > gralloc buffers (except on some devices that don't actually enforce that
> > like, like PowerVR).
> 
> I have a pile of unrooted Android devices on my desk from ICS onwards
> powered by QC, TI, nVidia SoCs (Samsung notably missing, but hey) that beg
> to differ...

We looked at this a couple years ago and had some success on TI (PowerVR) and NVIDIA. But it basically didn't work at all on Adreno as I recall. Maybe newer Adreno is more lenient. I wrote about it here: http://snorp.net/2011/12/16/android-direct-texture.html

I would much rather get the blessed SurfaceTexture version going if anything, though that would only work on about 20% of devices right now.

Comment 12

[Nicolas Silva [:nical]]

Comment on attachment 8455063 [details] [diff] [review]
Use gralloc for hw accelerated OMX buffers, v3

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

Wow that's a lot of code! I focused on the parts that affect layers (the wrapper thingy). r=me with the the comment about creating wrappers fixed.

::: content/media/plugins/ReferenceMediaResource.cpp
@@ +157,5 @@
> +  "\xa9\x74\x6f\x6f\x00\x00\x00\x1b\x64\x61\x74\x61\x00\x00\x00\x01" \
> +  "\x00\x00\x00\x00\x4c\x61\x76\x66\x35\x34\x2e\x32\x30\x2e\x34"
> +};
> +
> +ReferenceMediaResource* ReferenceMediaResource::sSingleton = nullptr;

Note that you can use StaticRefPtr<T> for this

::: gfx/layers/GraphicBufferWrapper.h
@@ +22,5 @@
> +
> +typedef uint32_t status_t;
> +#endif
> +
> +class GraphicBufferWrapper

Please add a small comment explaining why we need a wrapper.

This API would be less error-prone if there was no way to create an invalid Wrapper. You can do this by making the the constructor protected and using static methods that allocate and initialize the wrapper, only returning a non-null pointer if initialization did not fail. otherwise we will, sooner or later, forget to check the initialization somewhere.

::: gfx/layers/GraphicBufferWrapperGonk.cpp
@@ +7,5 @@
> +
> +#if ANDROID_VERSION >= 19
> +#define AS_FLATTENABLE(graphicBuffer) (graphicBuffer)
> +#else
> +#define AS_FLATTENABLE(graphicBuffer) ((Flattenable*)graphicBuffer.get())

shouldn't this be a static_cast?

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #11)
> I would much rather get the blessed SurfaceTexture version going if
> anything, though that would only work on about 20% of devices right now.

The decision is between landing the current set of patches or not landing them. Which do you prefer?

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #13)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #11)
> > I would much rather get the blessed SurfaceTexture version going if
> > anything, though that would only work on about 20% of devices right now.
> 
> The decision is between landing the current set of patches or not landing
> them. Which do you prefer?

The problem with landing these patches is that I don't know if we have done adequate testing. I would be more comfortable if we could get QA to run through an APK and see what jumps out.

Also, I haven't looked, but does this fallback gracefully if gralloc fails?

Comment 15

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #14)
> The problem with landing these patches is that I don't know if we have done
> adequate testing. I would be more comfortable if we could get QA to run
> through an APK and see what jumps out.

Yes, that's a good idea.

> Also, I haven't looked, but does this fallback gracefully if gralloc fails?

Yep.

Comment 16

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Build here: http://people.mozilla.org/~eflores/apk/fennec-34.0a1.en-US.android-arm.apk

Comment 17

[Aaron Train [:aaronmt]]

Have not forgotten about this. We'll be taking that APK through a tour soon. Can I get a summary of what this might fix and what this might break?

Comment 18

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Aaron Train [:aaronmt] from comment #17)
> Have not forgotten about this. We'll be taking that APK through a tour soon.
> Can I get a summary of what this might fix and what this might break?

Possible breakage:
 - Videos failing to play, maybe crashing
 - Leaks leading to crashes ~10-30s into a video

Possibly fixes:
 - Videos crashing in colour conversion code on some devices
 - Slow or stuttering video on some devices

Comment 19

[u421692]

I've tested on several devices and overall the experience with Fennec was very bad.
Please see the results below:

*LG Nexus 4 (Android 4.4.4)
Videos plays without problems, also plays in fullscreen: http://people.mozilla.org/~atrain/mobile/tests/media.html
Videos plays without problems, but does not play in fullscreen: http://www.dailymotion.com
Logs for dailymotion video in fullscreen: https://pastebin.mozilla.org/5855704

*Samsung Galaxy Tab 2 (Android 4.2.2)
Video starts playing, but there is no video, only sound

*Samsung Galaxy Nexus (Android 4.2.1)
Video starts playing, but there is no video, only sound

*Alcatel One Touch Android (Android 4.1.2)
Fennec crashes(no crash report) right after taping to play video
Logs for crash: https://pastebin.mozilla.org/5855731

*HTC Desire HD (Android 2.3.5)
Fennec crashes(no crash report) right after taping to play video
Logs for crash: https://pastebin.mozilla.org/5855741

*Samsung Galaxy Tab (Android 4.0.4)
Video starts playing, but there is no video, only sound; and sometimes Fennec crashes(no crash report)
Logs for no video and crash: https://pastebin.mozilla.org/5855753

Comment 20

[Aaron Train [:aaronmt]]

Attachment: log.log

Using my media page: http://people.mozilla.org/~atrain/mobile/tests/media.html

Samsung Galaxy S5 (Android 4.4.2)
LG Nexus 5 (Android 4.4.4)
HTC One (Android 4.4.2)

* H.264/MP4 (Video) tests → No video whatsoever (player throbber only), audio works

Log attached from LG Nexus 5

Comment 21

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Do these devices work with m-c?

Comment 22

[u421692]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #21)
> Do these devices work with m-c?

Yes, the same videos, on those devices work fine on latest Nightly.

Comment 23

[Aaron Train [:aaronmt]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #21)
> Do these devices work with m-c?

Yes.

Comment 24

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8455063 [details] [diff] [review]
Use gralloc for hw accelerated OMX buffers, v3

Cancelled review for now.