Android: SetCreateContextCallback

Android: SetCreateContextCallback

BUG=338338
TEST=
R=boliu@chromium.org,piman@chromium.org

[boliu, 2015-02-11 16:52:41]

https://codereview.chromium.org/920443003/diff/1/content/public/browser/andro...
File content/public/browser/android/synchronous_compositor.h (right):

https://codereview.chromium.org/920443003/diff/1/content/public/browser/andro...
content/public/browser/android/synchronous_compositor.h:45:
CreateContextCallback;
Return type should be a struct like below (since there is code that requires the
GLInProcessContext pointer):

struct ContextHolder {
  scoped_refptr<ContextProviderWebContext> context_provider;
  gpu::GLInProcessContext* context;
};

nit: blink::WebGraphicsContext3D::Attributes can be const&

And need lots of comments to explain arguments and return values.

[tfarina, 2015-02-15 20:47:17]

I have been having problems to compile for Android:

$ ./build/gyp_chromium -DOS=android -Goutput_dir=out_android
$ ninja -C out_android/Debug/ content_shell_apk
....
[6500/13168] SOLINK lib/libv8.cr.so
FAILED: if [ ! -e lib/libv8.cr.so -o ! -e lib/libv8.cr.so.TOC ]; then clang++
-shared -Wl,-z,now -Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,defs
-Wl,-z,noexecstack -fPIC -fuse-ld=gold
--gcc-toolchain=/home/tfarina/chromium/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/..
-Wl,--icf=safe -Wl,--no-undefined -target arm-linux-androideabi
--sysroot=/home/tfarina/chromium/src/third_party/android_tools/ndk//platforms/android-14/arch-arm
-nostdlib
-L/home/tfarina/chromium/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/libs/armeabi-v7a
-Wl,-shared,-Bsymbolic
/home/tfarina/chromium/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/lib/crtbegin_so.o
-m32 -Wl,--warn-shared-textrel -Wl,-O1 -Wl,--as-needed -Wl,--threads
-Wl,--thread-count=4 -o lib/libv8.cr.so -Wl,-soname=libv8.cr.so
@lib/libv8.cr.so.rsp && { readelf -d lib/libv8.cr.so | grep SONAME ; nm -gD -f p
lib/libv8.cr.so | cut -f1-2 -d' '; } > lib/libv8.cr.so.TOC; else clang++ -shared
-Wl,-z,now -Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,defs -Wl,-z,noexecstack
-fPIC -fuse-ld=gold
--gcc-toolchain=/home/tfarina/chromium/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/..
-Wl,--icf=safe -Wl,--no-undefined -target arm-linux-androideabi
--sysroot=/home/tfarina/chromium/src/third_party/android_tools/ndk//platforms/android-14/arch-arm
-nostdlib
-L/home/tfarina/chromium/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/libs/armeabi-v7a
-Wl,-shared,-Bsymbolic
/home/tfarina/chromium/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/lib/crtbegin_so.o
-m32 -Wl,--warn-shared-textrel -Wl,-O1 -Wl,--as-needed -Wl,--threads
-Wl,--thread-count=4 -o lib/libv8.cr.so -Wl,-soname=libv8.cr.so
@lib/libv8.cr.so.rsp && { readelf -d lib/libv8.cr.so | grep SONAME ; nm -gD -f p
lib/libv8.cr.so | cut -f1-2 -d' '; } > lib/libv8.cr.so.tmp && if ! cmp -s
lib/libv8.cr.so.tmp lib/libv8.cr.so.TOC; then mv lib/libv8.cr.so.tmp
lib/libv8.cr.so.TOC ; fi; fi
/home/tfarina/chromium/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../arm-linux-androideabi/bin/ld.gold:
warning: ignoring --threads:
/home/tfarina/chromium/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../arm-linux-androideabi/bin/ld.gold
was compiled without thread support
/home/tfarina/chromium/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../arm-linux-androideabi/bin/ld.gold:
warning: ignoring --thread-count:
/home/tfarina/chromium/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../arm-linux-androideabi/bin/ld.gold
was compiled without thread support
/home/tfarina/chromium/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../arm-linux-androideabi/bin/ld.gold:
error: treating warnings as errors
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[6500/13168] CXX obj/net/base/net.host_port_pair.o
ninja: build stopped: subcommand failed.

[tfarina, 2015-02-17 13:16:15]

FYI, I got the Android build working again.

I will make the changes you suggested and upload soon.

[tfarina, 2015-02-17 13:44:11]

Added ContextHolder struct. PTAL.

[boliu, 2015-02-17 16:50:43]

looks good so far

Are you intending to submit this as is, or trying to get feedback at this point?
I don't think we generally submit dead code to be used in the future.

https://codereview.chromium.org/920443003/diff/20001/content/public/browser/a...
File content/public/browser/android/synchronous_compositor.h (right):

https://codereview.chromium.org/920443003/diff/20001/content/public/browser/a...
content/public/browser/android/synchronous_compositor.h:59: //
|need_share_group_with_parent|.
Suggest putting the parameters next to the declaration, something like:

typedef base::Callback<ContextHolder(
      bool /* need_share_group_with_parent */,
      const blink::WebGraphicsContext3D::Attributes& /* attributes */,
      bool /* is_offscreen */)>
      CreateContextCallback;

Also suggest putting need_share_group_with_parent first (it replaces the current
"scoped_refptr<gpu::InProcessCommandBuffer::Service> service" parameter which is
first).

[tfarina, 2015-02-17 16:58:18]

I'm trying to get feedback, more like a guide of what to do in a pair
programming.

So you can see what I'm doing and my changes.

After you are happy with them, we can move forward to the next changes.

I have no idea yet the changes I will have to do to replace SetGpuService.

Hopefully with your guidance we will get there.

https://codereview.chromium.org/920443003/diff/20001/content/public/browser/a...
File content/public/browser/android/synchronous_compositor.h (right):

https://codereview.chromium.org/920443003/diff/20001/content/public/browser/a...
content/public/browser/android/synchronous_compositor.h:59: //
|need_share_group_with_parent|.
On 2015/02/17 16:50:43, boliu wrote:
> Suggest putting the parameters next to the declaration, something like:
> 
> typedef base::Callback<ContextHolder(
>       bool /* need_share_group_with_parent */,
>       const blink::WebGraphicsContext3D::Attributes& /* attributes */,
>       bool /* is_offscreen */)>
>       CreateContextCallback;
> 
> Also suggest putting need_share_group_with_parent first (it replaces the
current
> "scoped_refptr<gpu::InProcessCommandBuffer::Service> service" parameter which
is
> first).

Done.

[boliu, 2015-02-17 18:06:18]

On 2015/02/17 16:58:18, tfarina wrote:
> I'm trying to get feedback, more like a guide of what to do in a pair
> programming.
> 
> So you can see what I'm doing and my changes.
> 
> After you are happy with them, we can move forward to the next changes.

Cool.

> 
> I have no idea yet the changes I will have to do to replace SetGpuService.

Everything that needs gpu::InProcessCommandBuffer::Service right now will
instead call the callback you introduced. After that point content no longer
needs gpu::InProcessCommandBuffer::Service

> 
> Hopefully with your guidance we will get there.
> 
>
https://codereview.chromium.org/920443003/diff/20001/content/public/browser/a...
> File content/public/browser/android/synchronous_compositor.h (right):
> 
>
https://codereview.chromium.org/920443003/diff/20001/content/public/browser/a...
> content/public/browser/android/synchronous_compositor.h:59: //
> |need_share_group_with_parent|.
> On 2015/02/17 16:50:43, boliu wrote:
> > Suggest putting the parameters next to the declaration, something like:
> > 
> > typedef base::Callback<ContextHolder(
> >       bool /* need_share_group_with_parent */,
> >       const blink::WebGraphicsContext3D::Attributes& /* attributes */,
> >       bool /* is_offscreen */)>
> >       CreateContextCallback;
> > 
> > Also suggest putting need_share_group_with_parent first (it replaces the
> current
> > "scoped_refptr<gpu::InProcessCommandBuffer::Service> service" parameter
which
> is
> > first).
> 
> Done.

[tfarina, 2015-02-21 16:39:08]

Bo, I advanced a little here. Please, see my question below.

I hope to advance a little more this weekend.

Sorry, it has been hard to work on this in the week as I'm doing interviews.

PTAL!

https://codereview.chromium.org/920443003/diff/60001/android_webview/browser/...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/920443003/diff/60001/android_webview/browser/...
android_webview/browser/deferred_gpu_command_service.cc:28: return holder;
What I should put here?

[boliu, 2015-02-23 21:33:07]

https://codereview.chromium.org/920443003/diff/60001/android_webview/browser/...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/920443003/diff/60001/android_webview/browser/...
android_webview/browser/deferred_gpu_command_service.cc:28: return holder;
On 2015/02/21 16:39:08, tfarina wrote:
> What I should put here?

The this callback is replacing the CreateOffscreenContext and CreateContext
functions here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an...

So you are merging those functions into one here.

needs_share_group_with_parent replaces
coped_refptr<gpu::InProcessCommandBuffer::Service> service

And looks like I forgot the mem_limits parameter, so should add that as a
parameter to the callback as well.

Makes sense?

[tfarina, 2015-02-24 17:02:34]

Bo, thanks a lot for helping!

Please, take a look at the code I put in GetContextHolder() and my question
about needs_share_group_with_parent/service.

I hope to be moving in the right direction.

https://codereview.chromium.org/920443003/diff/60001/android_webview/browser/...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/920443003/diff/60001/android_webview/browser/...
android_webview/browser/deferred_gpu_command_service.cc:28: return holder;
On 2015/02/23 21:33:07, boliu wrote:
> On 2015/02/21 16:39:08, tfarina wrote:
> > What I should put here?
> 
> The this callback is replacing the CreateOffscreenContext and CreateContext
> functions here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an...
> 
> So you are merging those functions into one here.
> 
> needs_share_group_with_parent replaces
> coped_refptr<gpu::InProcessCommandBuffer::Service> service
> 
> And looks like I forgot the mem_limits parameter, so should add that as a
> parameter to the callback as well.
> 
> Makes sense?

Done.

https://codereview.chromium.org/920443003/diff/80001/android_webview/browser/...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/920443003/diff/80001/android_webview/browser/...
android_webview/browser/deferred_gpu_command_service.cc:30: bool
need_share_group_with_parent,
What I do with this?

[boliu, 2015-02-24 21:52:57]

https://codereview.chromium.org/920443003/diff/80001/android_webview/browser/...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/920443003/diff/80001/android_webview/browser/...
android_webview/browser/deferred_gpu_command_service.cc:30: bool
need_share_group_with_parent,
On 2015/02/24 17:02:34, tfarina wrote:
> What I do with this?

If true, pass g_service as the first parameter to GLInProcessContext::Create.
Otherwise pass null.

https://codereview.chromium.org/920443003/diff/80001/android_webview/browser/...
android_webview/browser/deferred_gpu_command_service.cc:59: holder.context =
context.get();
You already called context.Pass() 2 lines above, so it's null here now.

[tfarina, 2015-02-24 22:10:12]

https://codereview.chromium.org/920443003/diff/80001/android_webview/browser/...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/920443003/diff/80001/android_webview/browser/...
android_webview/browser/deferred_gpu_command_service.cc:30: bool
need_share_group_with_parent,
On 2015/02/24 21:52:57, boliu wrote:
> On 2015/02/24 17:02:34, tfarina wrote:
> > What I do with this?
> 
> If true, pass g_service as the first parameter to GLInProcessContext::Create.
> Otherwise pass null.

Done.

https://codereview.chromium.org/920443003/diff/80001/android_webview/browser/...
android_webview/browser/deferred_gpu_command_service.cc:59: holder.context =
context.get();
On 2015/02/24 21:52:57, boliu wrote:
> You already called context.Pass() 2 lines above, so it's null here now.

Done.

[boliu, 2015-02-26 01:32:54]

https://codereview.chromium.org/920443003/diff/100001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/920443003/diff/100001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:107:
CreateContextCallback callback) {
Next step is implement this.

Put this callback into the SynchronousCompositorFactoryImpl instance, and update
SynchronousCompositorFactoryImpl to use the callback instead of calling
CreateOffscreenContext or CreateContext.

[tfarina, 2015-02-28 18:56:57]

Bo, please take another look.

I was able to run content_unittests and android_webview_test on my Nexus 5!

content_unittests passed fine, but I didn't wait for android_webview tests to
finish.

Please, read/view
https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
carefully as I don't know if I get all the details right. See my question about
share_resources below.

I hope to be close now to a landable patch. Do you think we are getting there?

Thanks again, regards.

https://codereview.chromium.org/920443003/diff/100001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/920443003/diff/100001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:107:
CreateContextCallback callback) {
On 2015/02/26 01:32:54, boliu wrote:
> Next step is implement this.
> 
> Put this callback into the SynchronousCompositorFactoryImpl instance, and
update
> SynchronousCompositorFactoryImpl to use the callback instead of calling
> CreateOffscreenContext or CreateContext.

Done.

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:167:
callback_.Run(true, GetDefaultAttribs(), true, mem_limits).context)),
Do I need to manually create my Attributes? Do we need to pass share_resources
to callback?

[boliu, 2015-03-02 22:19:56]

If you have an android device, you can build and install the android_webview_apk
to test if this is working. And on the bots, watch for the "Instrumentation test
AndroidWebViewTest" step.

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(left):

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:213:
WrapContext(context.Pass()), debug_name);
Should pass the debug name to the callback as well. (But look at the
WebGraphicsContext3D below first, which might make this comment obsolete)

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:167:
callback_.Run(true, GetDefaultAttribs(), true, mem_limits).context)),
On 2015/02/28 18:56:57, tfarina wrote:
> Do I need to manually create my Attributes? Do we need to pass share_resources
> to callback?

Yes. And share_resource should be passed as part of attributes, not a separate
parameter.

The first parameter should be false.

This will cause a use after free error. ".context" is only a raw pointer into an
object owned by ".context_provider", so if you don't take hold of
".context_provider" here, then the memory gets destroyed, and ".context" just
becomes a dangling pointer. You should just return context_provider here
directly.

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:189:
.context),
ditto about ".context"

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:190:
attributes).release();
Hmm, there is no way to pass the ownership of a
WebGraphicsContext3DInProcessCommandBufferImpl object right now.

I wonder if we should update RendererBlinkPlatformImpl methods to return
WebGraphicsContext3DProvider instead of WebGraphicsContext3D. That might be a
big-ish change though.

(On the other hand, I don't understand the advantage of
WebGraphicsContext3DProvider over WebGraphicsContext3D, except to workaround
chromium layering issues like this.)

The other option is have the callback return WebGraphicsContext3D (instead of
ContextProvider, and then do the ContextProvider wrapping in this class where
necessary.

I'd lean towards option 2. Antoine, any suggestions here?

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:242:
gpu::GLInProcessContextSharedMemoryLimits()).context));
ditto about ".context"

You need to update VideoContextProvider constructor to take both the context
provider and context.

[tfarina, 2015-03-07 21:51:00]

Not very good yet because I have doubts in the changes I need to do around
WebGraphicsContext3D.

Please, help me address them.

Thanks,

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:167:
callback_.Run(true, GetDefaultAttribs(), true, mem_limits).context)),
On 2015/03/02 22:19:56, boliu wrote:
> On 2015/02/28 18:56:57, tfarina wrote:
> > Do I need to manually create my Attributes? Do we need to pass
share_resources
> > to callback?
> 
> Yes. And share_resource should be passed as part of attributes, not a separate
> parameter.
> 
> The first parameter should be false.
> 
> This will cause a use after free error. ".context" is only a raw pointer into
an
> object owned by ".context_provider", so if you don't take hold of
> ".context_provider" here, then the memory gets destroyed, and ".context" just
> becomes a dangling pointer. You should just return context_provider here
> directly.

Done.

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:190:
attributes).release();
On 2015/03/02 22:19:56, boliu wrote:
> Hmm, there is no way to pass the ownership of a
> WebGraphicsContext3DInProcessCommandBufferImpl object right now.
> 
> I wonder if we should update RendererBlinkPlatformImpl methods to return
> WebGraphicsContext3DProvider instead of WebGraphicsContext3D. That might be a
> big-ish change though.
> 
> (On the other hand, I don't understand the advantage of
> WebGraphicsContext3DProvider over WebGraphicsContext3D, except to workaround
> chromium layering issues like this.)
> 
> The other option is have the callback return gg (instead of
> ContextProvider, and then do the ContextProvider wrapping in this class where
> necessary.
> 
What we would do instead of:

holder.context_provider = webkit::gpu::ContextProviderInProcess::Create(  
gpu_blink::WebGraphicsContext3DInProcessCommandBufferImpl::WrapContext(context.Pass(),
attributes), std::string());

?

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:242:
gpu::GLInProcessContextSharedMemoryLimits()).context));
On 2015/03/02 22:19:56, boliu wrote:
> ditto about ".context"
> 
> You need to update VideoContextProvider constructor to take both the context
> provider and context.

Done.

[boliu, 2015-03-09 02:17:54]

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/920443003/diff/120001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:190:
attributes).release();
On 2015/03/07 21:51:00, tfarina wrote:
> On 2015/03/02 22:19:56, boliu wrote:
> > Hmm, there is no way to pass the ownership of a
> > WebGraphicsContext3DInProcessCommandBufferImpl object right now.
> > 
> > I wonder if we should update RendererBlinkPlatformImpl methods to return
> > WebGraphicsContext3DProvider instead of WebGraphicsContext3D. That might be
a
> > big-ish change though.
> > 
> > (On the other hand, I don't understand the advantage of
> > WebGraphicsContext3DProvider over WebGraphicsContext3D, except to workaround
> > chromium layering issues like this.)
> > 
> > The other option is have the callback return gg (instead of
> > ContextProvider, and then do the ContextProvider wrapping in this class
where
> > necessary.
> > 
> What we would do instead of:
> 
> holder.context_provider = webkit::gpu::ContextProviderInProcess::Create(  
>
gpu_blink::WebGraphicsContext3DInProcessCommandBufferImpl::WrapContext(context.Pass(),
> attributes), std::string());
> 
> ?
> 

Let me try to re-state the problem:

ContextProviderInProcess (CPIP) wraps
WebGraphicsContext3DInProcessCommandBufferImpl (WGC3D) which wraps
GLInProcessContext (IPC)

Currently the callback returns a scoped_refptr<CPIP> which owns the object, and
a non-owning pointer to the internal IPC object.

The problem in this method is we need to return an owning WGC3D pointer. This is
not possible because it's not possible to "unwrap" or "detach" a WGC3D once it's
wrapped in CPIP.

So I was proposing the callback should return an scoped_ptr<WGC3D> object and a
IPC pointer. Then wrapping a CPIP can happen here, instead of in the callback.

This mean that CPIP won't be moved to android_webview/, but to
content/browser/android/in_process/, which I think makes sense.

Let me know if this is not clear. And sorry I didn't see this problem earlier :/

https://codereview.chromium.org/920443003/diff/160001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/920443003/diff/160001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:94:
scoped_refptr<ContextProviderWebContext> context_provider_;
nit: This doesn't need to change