Closed Bug 1028053 Opened 6 years ago Closed 2 years ago

[Dolphin] H264 OMX Decoded frames cannot be composited

Categories

(Core :: Graphics: Layers, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
b2g-v1.4 --- affected

People

(Reporter: pehrsons, Unassigned, NeedInfo)

References

Details

Attachments

(4 files)

Prerequisites:
gecko-dev revision: dd65a59b3d09f437a7fa0f4ff8834b534e26c184
Patch: bug 1023125 blocks configuration of encoder
Patch: bug 1026933 blocks configuration of encoder
Patch: attachment 8423688 [details] [diff] [review] to get correct frames from camera

Steps to reproduce:
1. Go to http://firefoxos-public.comoyo.com/rtctest/single/
2. Hit start
3. Uncheck "Mute Audio"
4. When you hear audio coming through, the peerconnection has been established and video frames should be flowing through the H264 encoder and decoder

Expected result:
Video frames show in both video elements on the web page

Actual result:
Only one video element is showing frames on the web page (camera stream)
The problem here is twofold:

1. Compositor does not recognize the pixel format reported from hardware decoder

2. Pixel format reported from hardware decoder seems to be wrong

---

I'll try to elaborate on these two bullets here:


1. Compositor does not recognize the pixel format reported from hardware decoder

Hardware decoder is reporting 0x19 as the pixel format. For Spreadtrum that maps to HAL_PIXEL_FORMAT_YCbCr_420_SP (see system/core/include/system/graphics.h), which the compositor fails to interpret it here: http://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/GrallocTextureHost.cpp#23

Since it's an external format it should be fine to add it to the switch-case in GrallocTextureHost. That indeed makes the video frames show up, but people show up as blue smurfs (U and V channels switched), leading us to bullet nr 2.


2. Pixel format reported from hardware decoder seems to be wrong 

I investigated a bit where the pixel format came from and could see that SPRDAVCDecoder reports OMX_COLOR_FormatYUV420SemiPlanar (vendor/sprd/open-source/libs/omx_components/video/avc_sprd/sc8830/dec/SPRDAVCDecoder.cpp:(265 and 470).
In libstagefright, this then gets transformed into HAL_PIXEL_FORMAT_YCbCr_420_SP (frameworks/av/media/libstagefright/ACodec.cpp:578)
This is not part of Android stagefright but stems from a Spreadtrum patch with revision 66ce35af.

Seeing that U and V channels are swapped when displaying the decoded frames on screen, I changed frameworks/av/media/libstagefright/ACodec.cpp:578 to assign HAL_PIXEL_FORMAT_YCrCb_420_SP instead, and indeed it fixes the issue.
Here is a patch for the change I did to libstagefright/ACodec.cpp. I can also note that this fixes the problem under 1.), that the compositor does not recognize the format, since HAL_PIXEL_FORMAT_YCrCb_420_SP is already in the list of recognized formats. See http://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/GrallocTextureHost.cpp#23.

James, could you check on your side if commit 66ce35 in frameworks/av is doing the right thing or if it should be patched like this?
Flags: needinfo?(james.zhang)
Here's a patch to work around the issues mentioned under prerequisites in comment 0.
Assignee: nobody → ming.li
Flags: needinfo?(james.zhang)
Hi Andreas,

With you patch, there is still no video frame showing from youtube mpeg4 file playing. So bad. :( 
Just for your information.
For video rendering on compositor, vendor specific color format is handled by the followings. By the way, it seems better to handle this problem as part of Bug 931733.

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/GrallocTextureHost.cpp#39
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/GrallocTextureHost.cpp#67
Component: General → Graphics: Layers
Product: Firefox OS → Core
Changed to a correct component.
(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> By the way, it seems better to handle this problem as
> part of Bug 931733.
See comment 2, this could be a povb bug rather than a compositor bug. Let's keep it open until that's been settled by spreadtrum.
(In reply to Vincent Liu[:vliu][PTO:6/22~6/27] from comment #4)
> Hi Andreas,
> 
> With you patch, there is still no video frame showing from youtube mpeg4
> file playing. So bad. :( 
> Just for your information.
Ok, so we might have another issue on our hands here. 1.4 was working though, right?
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #2)
> Created attachment 8443332 [details] [diff] [review]
> 0001-Report-YCrCb-instead-of-YCbCr-for-YUV420SP-frames.patch
> 
> Here is a patch for the change I did to libstagefright/ACodec.cpp. I can
> also note that this fixes the problem under 1.), that the compositor does
> not recognize the format, since HAL_PIXEL_FORMAT_YCrCb_420_SP is already in
> the list of recognized formats. See
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/
> GrallocTextureHost.cpp#23.
> 
> James, could you check on your side if commit 66ce35 in frameworks/av is
> doing the right thing or if it should be patched like this?

Checked with our OMXCode owner, we don't support HAL_PIXEL_FORMAT_YCrCb_420_SP, it still has some issue now.
Comment on attachment 8443339 [details] [diff] [review]
Prerequisites patch

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

::: gfx/layers/GrallocImages.h
@@ +79,2 @@
>      HAL_PIXEL_FORMAT_YCrCb_420_SP_ADRENO    = 0x10A,
> +    HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED     = 0x18,

Gecko need this patch for Spreadtrum chipset, it's qcom chipset hard code.
We have landed this patch on v1.3t, but not land on v1.4.
You should land this patch on your git repo for dolphin.

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +805,3 @@
>      format->setInt32("level", level);
>      format->setInt32("bitrate-mode", bitrateMode);
>      format->setInt32("store-metadata-in-buffers", 0);

I will check this change with our OMX owner and I think it's qcom chipset hard code here.
Please ask gecko owner use "ro.moz.qcom" to control it.
blocking-b2g: --- → 1.4?
Depends on: 931733
Discussed with OMXCodec owner, I think gecko should support HAL_PIXEL_FORMAT_YCbCr_420_SP.
I'm not sure if this affects 1.4, it depends on if anything in 1.4 uses the H264 decoder. The WebRTC parts will land in 2.0 I believe.

As you can see in comment 1, U and V channels are switched when I let HAL_PIXEL_FORMAT_YCbCr_420_SP pass through the compositor. As I understand it, the pixel format value and texture just goes through gecko untouched. It reaches OpenGL which will display it on screen.
Sotaro, can you confirm that gecko doesn't do any readbacks or transformations for this case?

Both the H264 decoder and OpenGL are Spreadtrum parts, so unless gecko touches the pixel format or texture this seems to be a vendor issue.
Flags: needinfo?(sotaro.ikeda.g)
Here's a patch to enable gecko to show the frames from the h264 decoder as is.
James, please apply attachment 8443339 [details] [diff] [review] and attachment 8443977 [details] [diff] [review] and test it according to the STR in comment 0 on your end. Just to verify that we are both seeing the same issue.

While I still have my suspicions from comment 2, it will be easier for you to chase this one down if you're seeing the same symptoms as me.
Flags: needinfo?(james.zhang)
> As you can see in comment 1, U and V channels are switched when I let
> HAL_PIXEL_FORMAT_YCbCr_420_SP pass through the compositor. As I understand
> it, the pixel format value and texture just goes through gecko untouched. It
> reaches OpenGL which will display it on screen.
> Sotaro, can you confirm that gecko doesn't do any readbacks or
> transformations for this case?

In this use case, gecko does not do read back. gecko just bind to OpenGL and render it. But the code in Comment 5, need to know gralloc buffer's color format and makes decision of resultant color format and texutre tartget(LOCAL_GL_TEXTURE_EXTERNAL or LOCAL_GL_TEXTURE_2D)
Flags: needinfo?(sotaro.ikeda.g)
Kai-Zhen, could you run a test here with the prerequisites patch above and the new pixel format values James provided in bug 931733, comment 49?
Namely:
HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED     = 0x20,
HAL_PIXEL_FORMAT_YCbCr_420_SP           = 0x21,


To see if things behave differently. I won't be seeing a dolphin again for at least a month.
Flags: needinfo?(kli)
No need to test with these values then, since they were for Tarako. See bug 931733, comment 53.
Flags: needinfo?(kli)
Yeah, PIXEL_FORMAT should be the same as the one defined in system/core/include/system/graphics.h.
Please provide a video of the issue. Unable to decipher how bad the issue is.
Flags: needinfo?(pehrsons)
Flags: needinfo?(james.zhang) → needinfo?(ming.li)
> James, could you check on your side if commit 66ce35 in frameworks/av is
> doing the right thing or if it should be patched like this?

The owner is in Alan's team. I think it's OK because our android dolphin is MP release and firefox dolphin use the same code base.

commit 66ce35af731a7f2151f52459508cfb0c12ab76fc
Author: wenan.hu <wenan.hu@spreadtrum.com>
Date:   Tue Nov 19 13:54:06 2013 +0800

    Bug #241006 porting 4.1 sprd video codecs to 4.4
    
    [bug number  ]241006
    [root cause  ]
    [changes     ]
    [side effects]
    [reviewers   ]
Flags: needinfo?(alan.wang)
Hi Kai-Zhen,

Is this one the dup of bug 931733. It looks to me the same to bug 931733 per comment 5.
Flags: needinfo?(kli)
Ivan, I think this is a little bit more than bug 931733. After bug 931733 is landed, there could be specific patch for this bug.
Flags: needinfo?(kli)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #14)
> James, please apply attachment 8443339 [details] [diff] [review] and
> attachment 8443977 [details] [diff] [review] and test it according to the
> STR in comment 0 on your end. Just to verify that we are both seeing the
> same issue.
> 
> While I still have my suspicions from comment 2, it will be easier for you
> to chase this one down if you're seeing the same symptoms as me.


Sadly,here is no file of "WebrtcOMXH264VideoCodec.cpp" in gecko on the branch of telenor/v1.4 ,and i can't apply this patch.
Am i on the wrong branch? Is there anything else need to be done before applying the patch?
Flags: needinfo?(ming.li)
Per discussion with Solomon and Kai-zhen, this bug does not have dependency with bug 931733. Remove the dependency accordingly.

Moreover, tt looks to me the current patch is to add another pixel format specific for Dolphin so that I don't think the code should be landed in 1.4 branch for now. Please re-nominate it with real solution
blocking-b2g: 1.4? → ---
May be we can solve this issue in a different way.
No longer depends on: 931733
Assignee: ming.li → nobody
(In reply to Ming from comment #23)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #14)
> > James, please apply attachment 8443339 [details] [diff] [review] and
> > attachment 8443977 [details] [diff] [review] and test it according to the
> > STR in comment 0 on your end. Just to verify that we are both seeing the
> > same issue.
> > 
> > While I still have my suspicions from comment 2, it will be easier for you
> > to chase this one down if you're seeing the same symptoms as me.
> 
> 
> Sadly,here is no file of "WebrtcOMXH264VideoCodec.cpp" in gecko on the
> branch of telenor/v1.4 ,and i can't apply this patch.
> Am i on the wrong branch? Is there anything else need to be done before
> applying the patch?

After i switched to master branch and applied this patches, i can see both video elements showing video frames.
Andreas, now bug 931733 is landed. Can you still reproduce the same error?
(In reply to Ming from comment #26)
> After i switched to master branch and applied this patches, i can see both
> video elements showing video frames.

How are the colors? Everything good or are U and V swapped in the remote video stream? 

I'll have a new look at this when I'm back from vacation.
Flags: needinfo?(ming.li)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) (Back on July 28th) from comment #28)

> How are the colors? Everything good or are U and V swapped in the remote
> video stream? 
I don't how to check this . But these two videos looks the same and pictures in the right one has several secs delay compared to the left one.
Flags: needinfo?(ming.li)
If typical skin color appears as blue they are swapped. Sounds like they are correct.
Flags: needinfo?(pehrsons)
Didn't mean to clear that.
Flags: needinfo?(pehrsons)
Ming, I just tested according to comment 14.

This time I couldn't get my rtctest page working (couldn't see the remote side of the stream), but I tested a very recent v2.1 on the x3542 using appear.in. I used my laptop with a desktop build of the same gecko I used for B2G.

The frames show up correct locally on the device when coming from the camera, but after going through the hardware H264 encoder they appear to have U and V channels swapped. Thus, on my desktop they show up as blue.

See screenshot from desktop attached.
Flags: needinfo?(pehrsons) → needinfo?(ming.li)
Attachment #8485703 - Attachment description: Swapped U and V channels on remote side of dolphin OMX H264 encoder → Swapped U and V channels on remote side of dolphin OMX H264 encoder (text should be red)
Flags: needinfo?(ming.li)
Closing as we are not working on Firefox OS anymore.
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.