Open
Bug 711819
Opened 14 years ago
Updated 3 years ago
use neon fastpath in ConvertYCbCrToRGB32
Categories
(Core :: Graphics, defect)
Tracking
()
NEW
| blocking-basecamp | - |
People
(Reporter: gal, Unassigned)
Details
Attachments
(2 files, 3 obsolete files)
|
11.34 KB,
patch
|
Details | Diff | Splinter Review | |
|
19.71 KB,
patch
|
derf
:
review-
|
Details | Diff | Splinter Review |
Android has hardware support for YUV2RGB conversion with mirroring and rotation through the "copybit" device contained in libhardware.so. See hardware/libhardware/include/hardware/copybit.h for the API. We should use this for ConvertYCbCrToRGB32 and ScaleYCbCrToRGB32 since its hardware accelerated on some phones.
This is going to help B2G as well, obviously.
We need to figure out how this is implemented before deciding whether it would help us. We (already?) have code to unpack yuv with shaders during compositing, which is optimal wrt memory bandwidth.
Maybe we could use copybit when GL acceleration is not available. The question is whether there are many devices where that would be a win.
|
||
Comment 3•13 years ago
|
||
Hi all,
I found a chrome patch which optimizes YUV2RGB performance by NEON assembly, http://code.google.com/p/chromium/issues/detail?id=71403
In Cubevid apps, the major hotspot "FastConvertYUVToRGB32Row_C" reduced quite a lot from 23% to 3%.
BR
James
|
|
||
Comment 4•13 years ago
|
||
yuv2rgb neon patch
|
||
Updated•13 years ago
|
Attachment #684354 -
Attachment is patch: false
Attachment #684354 -
Attachment mime type: text/plain → application/x-tgz
|
|
||
Comment 5•13 years ago
|
||
Here's a version that applies well.
|
|
||
Comment 6•13 years ago
|
||
Comment on attachment 684409 [details] [diff] [review]
Patch
Randrom r? request to get this process kicked off.
Attachment #684409 -
Flags: review?(jones.chris.g)
|
|
||
Comment 7•13 years ago
|
||
Hi all,
You can test this path and use it in FFOS. It improves Cubevid performance.
BR
James
|
|
Reporter | |
Updated•13 years ago
|
Attachment #684409 -
Flags: review?(jones.chris.g) → review+
|
|
||
Comment 8•13 years ago
|
||
I expect that this patch will need to be properly added to the infrastructure like update.sh and the README in gfx/ycbcr as well.
|
|
Reporter | |
Comment 9•13 years ago
|
||
update.sh doesn't work for me any more, I think that machinery is out of date. I am ok with just patching over it.
|
|
Reporter | |
Comment 10•13 years ago
|
||
I would like to capture a stack to see why we end up here though. I would have thought we do this with GL.
|
|
Reporter | |
Comment 11•13 years ago
|
||
wow, ok, no wonder this is slow:
(gdb) bt 30
#0 mozilla::gfx::ConvertYCbCrToRGB32 (
y_buf=0x4316401c "\033\033\033\033\033\033\033\033", '\034' <repeats 14 times>, "\035\035\035\035", '\036' <repeats 28 times>, "\037\037\037\037 \037\037\037\037", '\036' <repeats 33 times>, "\034\033\032\032\032\032\032\032\032\032\032\033\033\034\034\035\033\032\031\031\031\031\031\031\031\031\031\032\032\032\032\032\030\027\025\025\025\026\026\030\030\031\031\031\030\027\026\025\022\021\020\020\016\r\017\024\027\031\033\036!$&''''()+,---,+**))()*++++,-/01100000023"...,
u_buf=0x4319c41c '\177' <repeats 103 times>, "~\200\205\214\225\230\230\230\221\204\203\201", '\177' <repeats 11 times>, "~zuss", 't' <repeats 45 times>, "uuvwxxxxxxxyyzz{{||{{z"...,
v_buf=0x431aa51c "\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\203\201\200", '\177' <repeats 15 times>"\200, \200\201\201\202\203\204\205\205\206\206\206\206\206\206\206\206\206\206\206\206\206\206\206\205\205\204\204\203\203\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202\202"..., rgb_buf=0x4c501000 "", pic_x=0, pic_y=0, pic_width=640, pic_height=360,
y_pitch=640, uv_pitch=320, rgb_pitch=2560, yuv_type=mozilla::gfx::YV12) at /Users/gal/workspace/B2G/gecko/gfx/ycbcr/yuv_convert.cpp:62
#1 0x40f17ee4 in gfxUtils::ConvertYCbCrToRGB (aData=..., aDestFormat=@0xbe93f91c, aDestSize=<value optimized out>, aDestBuffer=0x4c501000 "", aStride=2560)
at /Users/gal/workspace/B2G/gecko/gfx/thebes/gfxUtils.cpp:772
#2 0x40f27ec4 in mozilla::layers::PlanarYCbCrImage::GetAsSurface (this=0x469d2690) at /Users/gal/workspace/B2G/gecko/gfx/layers/ImageContainer.cpp:502
#3 0x40f40a64 in mozilla::layers::SharedPlanarYCbCrImage::GetAsSurface (this=0x4316401c) at /Users/gal/workspace/B2G/gecko/gfx/layers/ipc/ImageContainerChild.cpp:495
#4 0x40f284da in mozilla::layers::ImageContainer::GetCurrentAsSurface (this=0x46f23940, aSize=0xbe93f970) at /Users/gal/workspace/B2G/gecko/gfx/layers/ImageContainer.cpp:323
#5 0x40813fb8 in nsLayoutUtils::SurfaceFromElement (aElement=0x40379480, aSurfaceFlags=<value optimized out>)
at /Users/gal/workspace/B2G/gecko/layout/base/nsLayoutUtils.cpp:4490
#6 0x40e8026e in mozilla::WebGLContext::SurfaceFromElement<nsHTMLVideoElement> (cx=0x46e49b00, obj=<value optimized out>, self=0x439a5980, argc=<value optimized out>,
vp=0x42a38128) at /Users/gal/workspace/B2G/gecko/content/canvas/src/WebGLContext.h:1289
#7 mozilla::WebGLContext::TexImage2D<nsHTMLVideoElement> (cx=0x46e49b00, obj=<value optimized out>, self=0x439a5980, argc=<value optimized out>, vp=0x42a38128)
at /Users/gal/workspace/B2G/gecko/content/canvas/src/WebGLContext.h:863
#8 texImage2D (cx=0x46e49b00, obj=<value optimized out>, self=0x439a5980, argc=<value optimized out>, vp=0x42a38128)
at /Users/gal/workspace/B2G/objdir-gecko/dom/bindings/WebGLRenderingContextBinding.cpp:7238
#9 0x40e83872 in genericMethod (cx=0x46e49b00, argc=6, vp=0x42a38128) at /Users/gal/workspace/B2G/objdir-gecko/dom/bindings/WebGLRenderingContextBinding.cpp:9625
#10 0x41151466 in CallJSNative (cx=0x46e49b00, args=..., construct=js::NO_CONSTRUCT) at /Users/gal/workspace/B2G/gecko/js/src/jscntxtinlines.h:364
#11 InvokeKernel (cx=0x46e49b00, args=..., construct=js::NO_CONSTRUCT) at /Users/gal/workspace/B2G/gecko/js/src/jsinterp.cpp:369
#12 0x4114e518 in js::Interpret (cx=0x46e49b00, entryFrame=<value optimized out>, interpMode=<value optimized out>) at /Users/gal/workspace/B2G/gecko/js/src/jsinterp.cpp:2331
#13 0x41150d26 in js::RunScript (cx=0x46e49b00, script=<value optimized out>, fp=0x42a38038) at /Users/gal/workspace/B2G/gecko/js/src/jsinterp.cpp:326
#14 0x41152046 in InvokeKernel (cx=0x46e49b00, thisv=..., fval=..., argc=<value optimized out>, argv=0xbe9402d0, rval=0xbe940410)
at /Users/gal/workspace/B2G/gecko/js/src/jsinterp.cpp:381
#15 Invoke (cx=0x46e49b00, thisv=..., fval=..., argc=<value optimized out>, argv=0xbe9402d0, rval=0xbe940410) at /Users/gal/workspace/B2G/gecko/js/src/jsinterp.h:109
#16 Invoke (cx=0x46e49b00, thisv=..., fval=..., argc=<value optimized out>, argv=0xbe9402d0, rval=0xbe940410) at /Users/gal/workspace/B2G/gecko/js/src/jsinterp.cpp:414
#17 0x410f0bc6 in JS_CallFunctionValue (cx=0x46e49b00, objArg=<value optimized out>, fval=..., argc=1, argv=0xbe9402d0, rval=0xbe940410)
at /Users/gal/workspace/B2G/gecko/js/src/jsapi.cpp:5758
#18 0x40c2ec70 in nsXPCWrappedJSClass::CallMethod (this=0x46e2b880, wrapper=<value optimized out>, methodIndex=<value optimized out>, info_=0x425b6928,
nativeParams=0xbe9404d0) at /Users/gal/workspace/B2G/gecko/js/xpconnect/src/XPCWrappedJSClass.cpp:1432
#19 0x40c2c3f6 in nsXPCWrappedJS::CallMethod (this=0x475bae00, methodIndex=3, info=0x425b6928, params=<value optimized out>)
at /Users/gal/workspace/B2G/gecko/js/xpconnect/src/XPCWrappedJS.cpp:580
#20 0x40ecc624 in PrepareAndDispatch (self=0x47b72e40, methodIndex=<value optimized out>, args=0xbe94058c)
at /Users/gal/workspace/B2G/gecko/xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:105
#21 0x40ecbd8c in SharedStub () from /Users/gal/workspace/B2G/objdir-gecko/dist/bin/libxul.so
#22 0x40988a8a in nsEventListenerManager::HandleEventSubType (this=<value optimized out>, aListenerStruct=<value optimized out>, aListener=0x47b72e40, aDOMEvent=0x46e931f0,
aCurrentTarget=0x40379480, aPhaseFlags=6, aPusher=0xbe9406d0) at /Users/gal/workspace/B2G/gecko/content/events/src/nsEventListenerManager.cpp:893
#23 0x40988bc2 in nsEventListenerManager::HandleEventInternal (this=0x482be1f0, aPresContext=<value optimized out>, aEvent=0x475ce280, aDOMEvent=0xbe9406c0,
aCurrentTarget=0x40379480, aFlags=6, aEventStatus=0xbe9406c4, aPusher=0xbe9406d0) at /Users/gal/workspace/B2G/gecko/content/events/src/nsEventListenerManager.cpp:966
#24 0x40998064 in nsEventListenerManager::HandleEvent (this=<value optimized out>, aVisitor=<value optimized out>, aFlags=6,
---Type <return> to continue, or q <return> to quit---
aMayHaveNewListenerManagers=<value optimized out>, aPusher=0xbe9406d0) at /Users/gal/workspace/B2G/gecko/content/events/src/nsEventListenerManager.h:153
#25 nsEventTargetChainItem::HandleEvent (this=<value optimized out>, aVisitor=<value optimized out>, aFlags=6, aMayHaveNewListenerManagers=<value optimized out>,
aPusher=0xbe9406d0) at /Users/gal/workspace/B2G/gecko/content/events/src/nsEventDispatcher.cpp:184
#26 0x40998150 in nsEventTargetChainItem::HandleEventTargetChain (this=<value optimized out>, aVisitor=..., aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=false,
aPusher=0xbe9406d0) at /Users/gal/workspace/B2G/gecko/content/events/src/nsEventDispatcher.cpp:316
#27 0x409986d8 in nsEventDispatcher::Dispatch (aTarget=<value optimized out>, aPresContext=0x4aa87000, aEvent=0x475ce280, aDOMEvent=<value optimized out>,
aEventStatus=0xbe940750, aCallback=0x0, aTargets=0x0) at /Users/gal/workspace/B2G/gecko/content/events/src/nsEventDispatcher.cpp:634
#28 0x409988d0 in nsEventDispatcher::DispatchDOMEvent (aTarget=0x40379480, aEvent=0x475ce280, aDOMEvent=0x46e931f0, aPresContext=0x4aa87000, aEventStatus=0xbe940750)
at /Users/gal/workspace/B2G/gecko/content/events/src/nsEventDispatcher.cpp:694
#29 0x4093e930 in nsINode::DispatchEvent (this=0x40379480, aEvent=0x46e931f0, aRetVal=0xbe94078f) at /Users/gal/workspace/B2G/gecko/content/base/src/nsINode.cpp:1086
(More stack frames follow...)
(gdb)
|
|
Reporter | |
Updated•13 years ago
|
Summary: use android copybit driver in ConvertYCbCrToRGB32 and ScaleYCbCrToRGB32 → use neon fastpath in ConvertYCbCrToRGB32
|
|
Reporter | |
Comment 12•13 years ago
|
||
This is going to need texture streaming from webgl to be really fast. I hear thats close. James, want to try the patch bug 716859? It should avoid the CPU path altogether.
|
|
Reporter | |
Comment 13•13 years ago
|
||
Actually, no, the way the example works here, there will be still a CPU conversion. I will see if I can make the actual cubevid code smarter.
|
|
Reporter | |
Comment 14•13 years ago
|
||
Alright, we will need both patches. The texture streaming will avoid the CPU readback in WebGL and buy us some performance, and canvas will keep hitting the CPU path here for the time being.
|
|
Reporter | |
Comment 15•13 years ago
|
||
Comment on attachment 684409 [details] [diff] [review]
Patch
I am going to redo this patch a bit.
Attachment #684409 -
Flags: review+ → review?
(In reply to Andreas Gal :gal from comment #12)
> This is going to need texture streaming from webgl to be really fast. I hear
> thats close. James, want to try the patch bug 716859? It should avoid the
> CPU path altogether.
I don't think that patch will get you anything -- it doesn't (observabily, afaik) modify anything other than WebGL itself, and for B2G it effectively does the same thing it does today (for now -- it sets up the stage for something smarter).
Also, to texture from a video frame to webgl, you have to convert it to RGB -- we have no native YUV textures in WebGL, and we have no way to upload the raw YCbCr values to a texture (as we'd need two or three textures to do it, since the Cb/Cr channels are likely to be smaller than Y). So every time you do a texImage2D from a video file, you will hit ConvertYCbCrToRGB32 during the upload.
If the video is being decoded with the Gonk hardware decoder, the video frame will be a GonkIOSurfaceImage, which can be used as a texture directly (see ImageLayerOGL::RenderLayer). So can't we just make texImage2D copy from that texture to the destination texture? (Bonus points if we can somehow make texImage2D rebind to the GonkIOSurfaceImage texture directly without copying.)
|
|
Reporter | |
Comment 18•13 years ago
|
||
<3 comment 17. Even without that we get rid of the read back. Agree on CPU path. The patch needs a supports_neon. Please steal if you can. Until comment 17 we should land this.
|
|
Reporter | |
Comment 19•13 years ago
|
||
We could avoid webgl and use CSS if I could play the same source into multiple video elements without decoding multiple times.
(In reply to Andreas Gal :gal from comment #19)
> We could avoid webgl and use CSS if I could play the same source into
> multiple video elements without decoding multiple times.
You should be able to do that using MediaStreams, e.g.
video2.mozSrcObject = video1.mozCaptureStream();
I filed bug 814524 for comment #17.
|
|
Reporter | |
Comment 22•13 years ago
|
||
Can I do it for playback?
Not sure what you mean.
This demo works on desktop: http://people.mozilla.org/~roc/multi_stream.html
Doesn't seem to work on my Android phone though. Maybe OOM.
|
|
||
Comment 25•13 years ago
|
||
CLContext patch, which avoids readback data from GPU in Disable out-of-process case. It can share image memory cross-thread.
|
|
||
Comment 26•13 years ago
|
||
WEBGL_dynamic_texture is the most optimized solution to display video stream in 3D scene. But it rely on some low level extensions, including EGL_KHR_stream, EGL_KHR_stream_consumer_gltexture and GL_NV_EGL_stream_consumer_external. As I know, ARM/Mali will support it next year.
|
|
Reporter | |
Comment 27•13 years ago
|
||
|
|
Reporter | |
Comment 28•13 years ago
|
||
comment 23: roc, thats exactly what I need. Thanks.
|
|
Reporter | |
Comment 29•13 years ago
|
||
Assignee: nobody → gal
Attachment #684354 -
Attachment is obsolete: true
Attachment #684409 -
Attachment is obsolete: true
Attachment #684665 -
Attachment is obsolete: true
Attachment #684409 -
Flags: review?
|
|
Reporter | |
Updated•13 years ago
|
Attachment #684676 -
Flags: review?(roc)
Attachment #684676 -
Flags: review?(chris.double)
|
|
Reporter | |
Comment 30•13 years ago
|
||
One review will do. Whoever gets up first.
|
|
Reporter | |
Comment 31•13 years ago
|
||
I attached a CSS version of cubevid to bug 814737.
Comment on attachment 684676 [details] [diff] [review]
updated patch
Review of attachment 684676 [details] [diff] [review]:
-----------------------------------------------------------------
I'm not the right person to review this
Attachment #684676 -
Flags: review?(roc) → review?(tterribe)
|
||
Comment 33•13 years ago
|
||
Comment on attachment 684676 [details] [diff] [review]
updated patch
I defer to tim on this. My knowledge of neon is zero.
Attachment #684676 -
Flags: review?(chris.double)
|
||
Comment 34•13 years ago
|
||
Comment on attachment 684676 [details] [diff] [review]
updated patch
Review of attachment 684676 [details] [diff] [review]:
-----------------------------------------------------------------
This needs some work.
In the future, please generate patches with at least 8 lines of context (-U8).
::: gfx/ycbcr/Makefile.in
@@ +24,5 @@
> ycbcr_to_rgb565.cpp \
> $(NULL)
>
> +ifdef HAVE_ARM_NEON
> +SSRCS += yuv2rgb8888_neon.S
You should add this in the same section as yuv_row_arm.s. There's no reason to handle both files differently.
@@ +93,5 @@
> endif
> endif
> +
> +ifdef HAVE_ARM_NEON
> +yuv2rgb8888_neon.$(OBJ_SUFFIX): COMPILE_CFLAGS += $(ARM_NEON_CFLAGS)
So, this doesn't work. With this current patch on Android -mfpu=neon does not show up on the compile line and I get
/home/derf/src/android/mozilla-central/gfx/ycbcr/yuv_convert.cpp:93: undefined reference to `yuv420_2_rgb8888_neon'
/home/derf/src/android/mozilla-central/gfx/ycbcr/yuv_convert.cpp:97: undefined reference to `yuv422_2_rgb8888_neon'
You should use explicit directives in the .S file. See below.
::: gfx/ycbcr/yuv2rgb8888_neon.S
@@ +1,3 @@
> +// Copyright (c) 2011 ARM Limited. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
Do we need to update license.html for this? Where is the LICENSE file?
@@ +1,4 @@
> +// Copyright (c) 2011 ARM Limited. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
> +
In order for this to work with NEON detection, you need the following directives:
.arch armv7-a
.fpu neon
.object_arch armv4t
The last one is important for producing an executable which can run on all architectures. See gfx/ycbcr/yuv_row_arm.s for an example.
@@ +1,5 @@
> +// Copyright (c) 2011 ARM Limited. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
> +
> +#ifdef __ARM_NEON__
This doesn't get defined without -mfpu=neon, which you aren't using correctly (see above).
@@ +98,5 @@
> + const uint8_t *y_ptr,
> + const uint8_t *u_ptr,
> + const uint8_t *v_ptr,
> + int width,
> + int height); */
This prototype is incorrect.
@@ +109,5 @@
> + /* <> = height */
> + /* <> = y_pitch */
> + /* <> = uv_pitch */
> + /* <> = rgb_pitch */
> + .fnstart
There are a bunch of opportunities to shave a few cycles off in various places in this function. I assume that that is not the priority here.
@@ +310,5 @@
> + BGT loop_v_420
> +
> + VPOP {q4-q7}
> + POP {r4-r12, pc}
> + .fnend
Please include a .size directive so that gdb/valgrind etc., can identify this function on the callstack.
@@ +464,5 @@
> + BGT loop_v_422
> +
> + VPOP {q4-q7}
> + POP {r4-r12, pc}
> + .fnend
Please include a .size directive so that gdb/valgrind etc., can identify this function on the callstack.
@@ +466,5 @@
> + VPOP {q4-q7}
> + POP {r4-r12, pc}
> + .fnend
> +
> +#endif /* __ARM_NEON__ */
You need to include an ELF directive to indicate that this object does not require an executable stack in order to keep the linker from accidentally marking the stack executable for the entire program. See bug 752293 for an example.
::: gfx/ycbcr/yuv_convert.cpp
@@ +44,5 @@
> + int height,
> + int y_pitch,
> + int uv_pitch,
> + int rgb_pitch);
> +#endif /* MOZILLA_MAY_SUPPORT_MMX */
This comment is wrong.
@@ +88,5 @@
> +#ifdef MOZILLA_MAY_SUPPORT_NEON
> + if (supports_neon()) {
> + switch (yuv_type) {
> + case YV12:
> + yuv420_2_rgb8888_neon(rgb_buf, y_buf, u_buf, v_buf, pic_width, pic_height,
This function does not support pic_width<16 and is also incorrect if either pic_width or pic_height is odd. You need to detect those cases and fall back to the C version here (or fix the function to work correctly in these cases).
@@ +92,5 @@
> + yuv420_2_rgb8888_neon(rgb_buf, y_buf, u_buf, v_buf, pic_width, pic_height,
> + y_pitch, uv_pitch, rgb_pitch);
> + return;
> + case YV16:
> + yuv422_2_rgb8888_neon(rgb_buf, y_buf, u_buf, v_buf, pic_width, pic_height,
This function does not support pic_width<16 and is also incorrect if pic_width is odd. You need to detect those cases and fall back to the C version here (or fix the function to work correctly in these cases).
Attachment #684676 -
Flags: review?(tterribe) → review-
|
|
||
Updated•13 years ago
|
blocking-basecamp: --- → ?
|
||
Comment 35•13 years ago
|
||
Not sure why this was nominated but we can't block on it.
blocking-basecamp: ? → -
|
||
Comment 36•4 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.
Assignee: gal → nobody
|
||
Updated•3 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•