864017 - B2G performance on planar YCbCr regressed as GRALLOC_PLANAR_YCBCR path was omitted in new-layers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

(Branched from bug 861050)

In gfx/layers/client/ImageClient.cpp, in ImageClientSingle::UpdateImage, there is only a path from PLANAR_YCBCR and no path for GRALLOC_PLANAR_YCBCR. But on B2G, we have a GRALLOC_PLANAR_YCBCR. So we don't take any fast path there and fall back to the slow path of calling GetAsSurface and doing things in software.

Comment 1

[Maire Reavy [:mreavy]]

Thanks for opening this bug, Benoit, and for flagging this as a likely issue with current B2G WebRTC video performance.

Benoit & Milan -- Getting a solution to this will be very important to achieving usable video performance on B2G for WebRTC.  Can we make this bug a high priority to understand and solve?

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

That sounds like a reasonable choice of next thing for me to work on.

Do you have a testcase for me to test against?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

See the patches on bug 861050.  You can try simple WebRTC tests like from the webrtc-landing demo page (perhaps http://nightly-gupshup.herokuapp.com/).  The pc_test.html page is a heavy test, in that it's a two-way call within the same page (basically two calls at once).

For perf monitoring, this is what slee pasted:

    I just can use perf to profile the cpu usage on B2G.
    Here are the steps.
    1. download "profiling" branch of git://github.com/jld/B2G.git
    2. remove out and gecko-output folder
    3. modify .userconfig
        * export B2G_PROFILING=1
        * If you specify GECKO_PATH and GECKO_OBJDIR in .userconfig, remember use absolute path.
    4. apply the attached patch
    5. disable elf-hack (I am not sure if it is necessary but I disable it.)
    6. Build you project. Then you can use "./run-perf.sh record -a -g" to capture and "./run-perf.sh report" to get report.
     
    For the updated information, you can check bug 831611 - Get perf working on b2g.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: crash stack --- again the Send__delete__ crash

Got a build with today's mozilla-central and the patch queue https://bugzilla.mozilla.org/attachment.cgi?id=739521

Unfortunately, upon joining a chat in 'gupshup', this consistently runs into the known Send__delete__ crash we have been struggling with, see attached stack; this is the same crash as in bug 864598.

Comment 5

[Bas Schouten (:bas.schouten)]

I have a patch for this but I can't seem to be able to test this. When I go to the suggested WebRTC test site I never get a prompt that let's me allow the page access to my Unagi's camera.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Bas: go to the same page on your desktop computer, log in on both your computer and your unagi, and invite/call your unagi username from your computer session.

Comment 7

[Bas Schouten (:bas.schouten)]

(In reply to Benoit Jacob [:bjacob] from comment #6)
> Bas: go to the same page on your desktop computer, log in on both your
> computer and your unagi, and invite/call your unagi username from your
> computer session.

Nothing happens.. my phone just sits there as far as I can tell. I'll try again.

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Bas Schouten (:bas.schouten) from comment #7)
> (In reply to Benoit Jacob [:bjacob] from comment #6)
> > Bas: go to the same page on your desktop computer, log in on both your
> > computer and your unagi, and invite/call your unagi username from your
> > computer session.
> 
> Nothing happens.. my phone just sits there as far as I can tell. I'll try
> again.

I get the call, I accept, it opens, but it shows no video with either Local or Remote, and it doesn't seem to ask me if I want to enable my video.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Not sure what's going on; if you can get signaling:5 logs from the desktop that would help a lot (and I suspect be easier than getting them from the Unagi). You may want to use a debug build on desktop; more debugging info is available.

If the issue is on the unagi side, you may need to ask the B2G guys how to get logs out.  Interesting logs would include signaling:5,mediamanager:5,mtransport:5

Also: it helps when debugging to make sure the two devices (unagi and desktop) are on the same subnet.

Comment 10

[Bas Schouten (:bas.schouten)]

So with the right patch queue I got things running. How I'm running into the cycle collector off-main-thread crash. I have seen bug 825110 but I'm not sure how to proceed. I still haven't been able to test this, in the meanwhile, I'll put my patch up for review here soon :)

Comment 11

[Bas Schouten (:bas.schouten)]

Attachment: Forward the surface descriptor for GrallocPlanarYCbCrImage

This patch, together with the patch in bug 866521, should address this bug. I've been unable to test this due to the cycle collector crash though.

Comment 12

[Chiajung Hung [:chiajung]]

(In reply to Bas Schouten (:bas.schouten) from comment #10)
> So with the right patch queue I got things running. How I'm running into the
> cycle collector off-main-thread crash. I have seen bug 825110 but I'm not
> sure how to proceed. I still haven't been able to test this, in the
> meanwhile, I'll put my patch up for review here soon :)

As talked on IRC, please update the patch to newer version and try again. It should work. If you find any problem in this part again, just let me know. :)

Comment 13

[Bas Schouten (:bas.schouten)]

After applying some more patches sent by Chiajung it's now crashing here:

0x414aebfa in nsMainThreadPtrHolder (this=0x4badd0b0, ptr=0x4553e818, 
    strict=<value optimized out>) at ../../dist/include/nsProxyRelease.h:116
116	    MOZ_ASSERT(!mStrict || NS_IsMainThread());
(gdb) bt
#0  0x414aebfa in nsMainThreadPtrHolder (this=0x4badd0b0, ptr=0x4553e818, 
    strict=<value optimized out>) at ../../dist/include/nsProxyRelease.h:116
#1  0x414aecfa in GetPreviewStreamTask (this=0x4865a920, aSize=<value optimized out>, 
    onSuccess=0x4553e818, onError=0x4553e824)
    at /home/bas/Dev/mozilla-central/dom/camera/CameraControlImpl.h:224
#2  mozilla::CameraControlImpl::GetPreviewStream (this=0x4865a920, 
    aSize=<value optimized out>, onSuccess=0x4553e818, onError=0x4553e824)
    at /home/bas/Dev/mozilla-central/dom/camera/CameraControlImpl.cpp:333
#3  0x412070fa in mozilla::MediaEngineWebRTCVideoSource::Start (this=0x4553e800, 
    aStream=0x4c3a04a0, aID=1)
    at /home/bas/Dev/mozilla-central/content/media/webrtc/MediaEngineWebRTCVideo.cpp:330
#4  0x4132adce in mozilla::MediaOperationRunnable::Run (this=0x4a5aa8a0)
    at /home/bas/Dev/mozilla-central/dom/media/MediaManager.h:296
#5  0x41bd3772 in nsThread::ProcessNextEvent (this=0x4b5d3b70, 
    mayWait=<value optimized out>, result=0x4c1ffe87)
    at /home/bas/Dev/mozilla-central/xpcom/threads/nsThread.cpp:627
#6  0x41b9a088 in NS_ProcessNextEvent (thread=0x6a, mayWait=true)
    at /home/bas/Dev/mozilla-central/objdir-gonk/xpcom/build/nsThreadUtils.cpp:238
#7  0x41bd3dc6 in nsThread::ThreadFunc (arg=<value optimized out>)
    at /home/bas/Dev/mozilla-central/xpcom/threads/nsThread.cpp:265
#8  0x4086f2ea in _pt_root (arg=<value optimized out>)
    at /home/bas/Dev/mozilla-central/nsprpub/pr/src/pthreads/ptthread.c:204
#9  0x400cce18 in __thread_entry (func=0x4086f249 <_pt_root>, arg=0x4ba6b680, 
    tls=<value optimized out>) at bionic/libc/bionic/pthread.c:217

Comment 14

[Chiajung Hung [:chiajung]]

Hmm...it seems like I have to check which thread Camera API expect its function to be called...I will update Bug 825110 as soon as I checked that.

Comment 15

[Chiajung Hung [:chiajung]]

It seems Camera callback are hold by nsMainThreadPtrHolder<T> now, which means I have to post all function call of camera to MainThread...I don't know when these are changed to such state, but I will fix my part later.

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 742872 [details] [diff] [review]
Forward the surface descriptor for GrallocPlanarYCbCrImage

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

Oh hey, I didn't suspect it would be so simple.

Comment 17

[Chiajung Hung [:chiajung]]

One question here:

PlanarYCbCrImage should means a I420(Planar YUV) format image but the GraphicBuffer allocated in:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages.cpp#43

is YV12 format, which is PlanarYVU format. I think it is easy to fix by swap UV channel pointers before memcpy.

Comment 18

[Chiajung Hung [:chiajung]]

And this cause crash here:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/TextureHostOGL.cpp#599

Since YV12 format is 0x32315659. You should add this (either the literal or include the correct header) into the list I think. If this patch is going to merge and close, I can file another bug for enable this feature for WebRTC.

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Flagging Bas so he can't forget about these two comments ;-)

Comment 20

[Bas Schouten (:bas.schouten)]

(In reply to Chiajung Hung [:chiajung] from comment #18)
> And this cause crash here:
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/
> TextureHostOGL.cpp#599
> 
> Since YV12 format is 0x32315659. You should add this (either the literal or
> include the correct header) into the list I think. If this patch is going to
> merge and close, I can file another bug for enable this feature for WebRTC.

See https://bugzilla.mozilla.org/show_bug.cgi?id=864017#c11. This is the reason you need the patch in bug 866521.

Comment 21

[Bas Schouten (:bas.schouten)]

(In reply to Chiajung Hung [:chiajung] from comment #17)
> One question here:
> 
> PlanarYCbCrImage should means a I420(Planar YUV) format image but the
> GraphicBuffer allocated in:
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages.cpp#43
> 
> is YV12 format, which is PlanarYVU format. I think it is easy to fix by swap
> UV channel pointers before memcpy.

This code didn't change with the refactor. Am I correct to understand this was already broken?

Comment 22

[Timothy B. Terriberry (:derf)]

(In reply to Chiajung Hung [:chiajung] from comment #17)
> PlanarYCbCrImage should means a I420(Planar YUV) format image but the
> GraphicBuffer allocated in:
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages.cpp#43
> 
> is YV12 format, which is PlanarYVU format. I think it is easy to fix by swap
> UV channel pointers before memcpy.

Um, YV12 has the planes in the order Y', Cb, Cr. It is generally the format output by all of our software codecs. That should really be the same order used by PlanarYCbCrImage.

There is quite a bit of confusion over what the "U" and "V" planes of "YUV" actually correspond to (which is why we label things YCbCr in our own code).

Comment 23

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58853247c43b

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58853247c43b

Comment 25

[Chiajung Hung [:chiajung]]

(In reply to Bas Schouten (:bas.schouten) from comment #11)
> Created attachment 742872 [details] [diff] [review]
> Forward the surface descriptor for GrallocPlanarYCbCrImage
> 
> This patch, together with the patch in bug 866521, should address this bug.
> I've been unable to test this due to the cycle collector crash though.

Ok, I see. I did not notice that one.

Comment 26

[Chiajung Hung [:chiajung]]

(In reply to Timothy B. Terriberry (:derf) from comment #22)
> (In reply to Chiajung Hung [:chiajung] from comment #17)
> > PlanarYCbCrImage should means a I420(Planar YUV) format image but the
> > GraphicBuffer allocated in:
> > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages.cpp#43
> > 
> > is YV12 format, which is PlanarYVU format. I think it is easy to fix by swap
> > UV channel pointers before memcpy.
> 
> Um, YV12 has the planes in the order Y', Cb, Cr. It is generally the format
> output by all of our software codecs. That should really be the same order
> used by PlanarYCbCrImage.
> 
> There is quite a bit of confusion over what the "U" and "V" planes of "YUV"
> actually correspond to (which is why we label things YCbCr in our own code).
 
The naming is confusing for me, too.

According to 
http://www.fourcc.org/yuv.php

You can search for I420, and you will find 
YV12 	0x32315659 	12 	8 bit Y plane followed by 8 bit 2x2 subsampled V and U planes.
I420 	0x30323449 	12 	8 bit Y plane followed by 8 bit 2x2 subsampled U and V planes.

That's why I think the UV plane should be swapped.

Comment 27

[Chiajung Hung [:chiajung]]

(In reply to Bas Schouten (:bas.schouten) from comment #21)
> (In reply to Chiajung Hung [:chiajung] from comment #17)
> > One question here:
> > 
> > PlanarYCbCrImage should means a I420(Planar YUV) format image but the
> > GraphicBuffer allocated in:
> > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/GrallocImages.cpp#43
> > 
> > is YV12 format, which is PlanarYVU format. I think it is easy to fix by swap
> > UV channel pointers before memcpy.
> 
> This code didn't change with the refactor. Am I correct to understand this
> was already broken?

As comment 26 says, I think YV12 is different from I420 from definition.