892934 - No MP4 video displayed if colour depth set to 24; video displayed when set to 16

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[wbsecg1]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130627185035

Steps to reproduce:

I test firefox for android nightly build version on my 2 HTC device, mozilla central 20130704 apk can play mp4, but 0705 can't, no pitcure displayed, only audio. Finally i find the issue is from this git commit: 6288fc96437b764405378f523b73aca7caced013. It checks color depth, can be 24 or 16. If I force it return 16, gecko can play mp4 again(test on both our gecko and firefox for android). Our gecko build with code before git commit e828601b893e6afbae5caa19e58c3ee76cce3c58 can play mp4 well, with color depth 24bpp. But after merging May's code, only 16 bpp can play mp4.
So I think there must be a mistake in Aprils commits. In April, there are huge changes in gfx, the mistake may be in gfx.


Actual results:

no frame displayed if using 24 bpp. 16 bpp works fine.


Expected results:

24 bpp should play mp4 too.

Comment 1

[wbsecg1]

HTC One

Comment 2

[Aaron Train [:aaronmt]]

This is w

Comment 3

[Aaron Train [:aaronmt]]

This is what a few others in channel were seeing the other day, CC'ed a few.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

I'm assuming setting gfx.android.rgb16.force to true makes the problem go away?

Comment 5

[Aaron Train [:aaronmt]]

(In reply to Milan Sreckovic [:milan] from comment #4)
> I'm assuming setting gfx.android.rgb16.force to true makes the problem go
> away?

Confirmed. HTC One (Android 4.2.2), Nightly (07/15).

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Edwin, is it possible we're not doing a conversion when 16-bit video comes in on Android, assuming it's going to be a 16-bit buffer (which it was until bug 803299 got done?)

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Aaron, can you clarify the change in summary?  Is it any video format inside of the HTML5 video tag, is it mp4 videos "delivered somehow" which I assumed from the original summary, or some combination?

Comment 8

[Aaron Train [:aaronmt]]

Only MP4 as far as I can see thus far, I'll re-summarize.

Comment 9

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

Yes, I assumed at the time that the output format would always be RGB565. All that needs to happen is that the OMX plugin be made aware of it at [1] and [2]. Maybe [3].

Also the ImageBufferCallback [4] needs to be able to construct RGB24 images. 

[1] https://mxr.mozilla.org/mozilla-central/source/media/omx-plugin/OmxPlugin.cpp#734
[2] https://mxr.mozilla.org/mozilla-central/source/media/omx-plugin/OmxPlugin.cpp#712
[3] https://mxr.mozilla.org/mozilla-central/source/media/omx-plugin/OmxPlugin.cpp#339
[4] https://mxr.mozilla.org/mozilla-central/source/content/media/plugins/MediaPluginReader.cpp#365

Comment 10

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

I can't reproduce this on trunk on my HTC One X. Could you post a logcat?

Comment 11

[Aaron Train [:aaronmt]]

Attachment: HTC One (Android 4.2.2) logcat

Comment 12

[cajbir (:cajbir)]

I had to set the pref mentioned in comment 4 when doing testing for another bug on the Sony Xperia Z. Prior to setting that pref I just got a black video image so it seems to reproduce on that. This was on trunk a few days ago.

Comment 13

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

Hm. Not quite as quick and easy as I thought. Android's ColorConverter doesn't support conversions to RGB24 on either of the two phones I have on hand. Will probably just have to support RGB565 surfaces in 24 bit mode somehow.

Comment 14

[wbsecg1]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #10)
> I can't reproduce this on trunk on my HTC One X. Could you post a logcat?

(In reply to Edwin Flores [:eflores] [:edwin] from comment #13)
> Hm. Not quite as quick and easy as I thought. Android's ColorConverter
> doesn't support conversions to RGB24 on either of the two phones I have on
> hand. Will probably just have to support RGB565 surfaces in 24 bit mode
> somehow.

htc one use qcom gpu, but one x use nvidia gpu. color format support is different. but i think it doesn't matter, because before april, 24bpp works fine, use colorconverter too. and omxplugin changes little after april.

Comment 15

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

(In reply to wbsecg1 from comment #14)
> htc one use qcom gpu, but one x use nvidia gpu. color format support is
> different. but i think it doesn't matter, because before april, 24bpp works
> fine, use colorconverter too. and omxplugin changes little after april.

The ColorConverter path was only ever using RGB565. I'm guessing somebody broke the 565->888 conversion during the gfx refactor.

Comment 16

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

Works properly for me if I force the TexImage2D path here: https://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContext.cpp#2146

Otherwise it takes the TexSubImage2D path, which fails with error GL_INVALID_OPERATION. I'm guessing it's because the matching TexImage2D was called with a 24 bit format.

Comment 17

[Matt Woodrow (:mattwoodrow)]

That seems like a likely cause.

I can't see how we call TexImage2D with the wrong format though, unless we send an RGB24 surface the first time, and then send 565.

Getting a stack trace for the Tex{Sub}Image2D calls and checking what the current bound texture is might let you narrow that down.

Comment 18

[Timothy B. Terriberry (:derf)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> I can't see how we call TexImage2D with the wrong format though, unless we
> send an RGB24 surface the first time, and then send 565.

Not as unlikely as you think. The first few video frames are often decoded without knowing the size of the video element (because layout hasn't fed back this information yet), so they can take different code paths (e.g., for software conversion, simultaneous conversion+scaling is disabled until the destination size is known).

Comment 19

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

Looks like in [1] we don't differentiate between different RGB packings. Writing a patch now to pass the ImageFormat into *::CreateTextureImage.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderEGL.cpp#1117

Comment 20

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

Attachment: 892934-createtextureimage-imageformat.patch

Comment 21

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

Comment on attachment 780769 [details] [diff] [review]
892934-createtextureimage-imageformat.patch

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

::: gfx/gl/GLContextProviderGLX.cpp
@@ +939,5 @@
>      virtual already_AddRefed<TextureImage>
>      CreateTextureImage(const nsIntSize& aSize,
>                         TextureImage::ContentType aContentType,
>                         GLenum aWrapMode,
> +                       TextureImage::Flags aFlags = TextureImage::NoFlags

Missed a comma here. Known thing.

Comment 22

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

Attachment: 892934-createtextureimage-imageformat.patch

Missed platform-specific craziness.

Comment 23

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 780790 [details] [diff] [review]
892934-createtextureimage-imageformat.patch

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

::: gfx/gl/GLContext.h
@@ +777,5 @@
>      CreateTextureImage(const nsIntSize& aSize,
>                         TextureImage::ContentType aContentType,
>                         GLenum aWrapMode,
> +                       TextureImage::Flags aFlags = TextureImage::NoFlags,
> +                       TextureImage::ImageFormat aImageFormat = gfxASurface::ImageFormatUnknown);

Comment that this format is a hint, and isn't necessarily used.

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +503,5 @@
> +  }
> +
> +  sp<GraphicBuffer> buffer =
> +    GrallocBufferActor::GetFrom(aDescriptor.get_SurfaceDescriptorGralloc());
> +  *aContent = ImageFormatForPixelFormat(buffer->getPixelFormat());

Does this compile? Seems like you should name the parameters in this function...

Prefer aFormat over aContent.

::: gfx/layers/ipc/ShadowLayers.h
@@ +431,5 @@
> +  // And again, for the image format
> +  static gfxImageFormat
> +  GetDescriptorSurfaceImageFormat(const SurfaceDescriptor& aDescriptor,
> +                                  OpenMode aMode,
> +                                  gfxASurface** aSurface);

Either comment that this can return ImageFormatUnknown, or assert that it doesn't.

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +181,5 @@
>  
>    if (!mTexture ||
>        (mTexture->GetSize() != size && !aOffset) ||
> +      mTexture->GetContentType() != surf.ContentType() ||
> +      mTexture->GetImageFormat() != surf.ImageFormat()) {

If surf.ImageFormat() is returning Unknown, mTexture->GetImageFormat() will reuturn the actual chosen one, right?

Won't this lead to us recreating textures much too often?

It might be better if we could assert that surf.ImageFormat never returns unknown.

Comment 24

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #23)
> ::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
> @@ +503,5 @@
> > +  }
> > +
> > +  sp<GraphicBuffer> buffer =
> > +    GrallocBufferActor::GetFrom(aDescriptor.get_SurfaceDescriptorGralloc());
> > +  *aContent = ImageFormatForPixelFormat(buffer->getPixelFormat());
> 
> Does this compile? Seems like you should name the parameters in this
> function...
> 
> Prefer aFormat over aContent.

Er, no it doesn't. Knee-jerk oh-no-try-failure-patch-it-quick-and-reupload-and-see-if-reviewer-notices.

> ::: gfx/layers/opengl/TextureHostOGL.cpp
> @@ +181,5 @@
> >  
> >    if (!mTexture ||
> >        (mTexture->GetSize() != size && !aOffset) ||
> > +      mTexture->GetContentType() != surf.ContentType() ||
> > +      mTexture->GetImageFormat() != surf.ImageFormat()) {
> 
> If surf.ImageFormat() is returning Unknown, mTexture->GetImageFormat() will
> reuturn the actual chosen one, right?

Yes.

> Won't this lead to us recreating textures much too often?

...yes.

> It might be better if we could assert that surf.ImageFormat never returns
> unknown.

Might be. Does this path ever get surfaces that are non-ImageSurfaces?

Comment 25

[Matt Woodrow (:mattwoodrow)]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #24)
> 
> > It might be better if we could assert that surf.ImageFormat never returns
> > unknown.
> 
> Might be. Does this path ever get surfaces that are non-ImageSurfaces?

I think it can for X11 surfaces at least.

Comment 26

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #25)
> I think it can for X11 surfaces at least.

Just to make sure I ran a try push asserting that the image format is never unknown:
https://tbpl.mozilla.org/?tree=Try&rev=9e79c1fc615b

Looks like we should be good.

Comment 27

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

Attachment: 892934-createtextureimage-imageformat.patch

Try: https://tbpl.mozilla.org/?tree=Try&rev=4fb5fd909459

Comment 28

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/49b181b3d59c

Comment 29

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

Comment on attachment 786083 [details] [diff] [review]
892934-createtextureimage-imageformat.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): layers refactor
User impact if declined: No h264 playback for significant number of Android devices.
Testing completed (on m-c, etc.): Try green; WFM on device.
Risk to taking this patch (and alternatives if risky): Small chance of breakage/crashing across multiple platforms, but fixed everything try picks up.
String or IDL/UUID changes made by this patch: None

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49b181b3d59c

Comment 31

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/a4dbc38c7510

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Please don't use [leave open] if the only reason is for a branch uplift. It screws with the queries (also having it resolved with the target milestone unset hurts).