Closed Bug 892934 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Audio/Video, defect)

25 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 --- unaffected
firefox25 + fixed
firefox26 + fixed
fennec 25+ ---

People

(Reporter: wbsecg1, Assigned: eflores)

References

()

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 2 obsolete files)

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.
HTC One
OS: Linux → Android
Priority: -- → P3
Hardware: x86_64 → ARM
Component: Graphics, Panning and Zooming → Video/Audio
Product: Firefox for Android → Core
Version: Firefox 25 → unspecified
This is w
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Priority: P3 → --
Version: unspecified → 25 Branch
This is what a few others in channel were seeing the other day, CC'ed a few.
Blocks: 803299
Assignee: nobody → blassey.bugs
Assignee: blassey.bugs → chrislord.net
I'm assuming setting gfx.android.rgb16.force to true makes the problem go away?
(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).
Status: NEW → ASSIGNED
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?)
Flags: needinfo?(edwin)
Summary: can not play mp4 if color depth set to 24 but fine if set to 16 → No HTML5 video displayed if colour depth set to 24; video displayed when set to16
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?
Only MP4 as far as I can see thus far, I'll re-summarize.
Summary: No HTML5 video displayed if colour depth set to 24; video displayed when set to16 → No MP4 video displayed if colour depth set to 24; video displayed when set to 16
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
Flags: needinfo?(edwin)
Assignee: chrislord.net → edwin
tracking-fennec: ? → 25+
I can't reproduce this on trunk on my HTC One X. Could you post a logcat?
Flags: needinfo?(aaron.train)
Flags: needinfo?(aaron.train)
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.
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.
(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.
(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.
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.
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.
(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).
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 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.
Missed platform-specific craziness.
Attachment #780769 - Attachment is obsolete: true
Attachment #780769 - Flags: review?(matt.woodrow)
Attachment #780790 - Flags: review?(matt.woodrow)
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.
(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?
(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.
(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.
Attachment #786083 - Flags: review?(matt.woodrow) → review+
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
Attachment #786083 - Flags: approval-mozilla-aurora?
Attachment #786083 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/a4dbc38c7510
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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).
Whiteboard: [leave open]
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.