AHardwareBuffer-based GpuMemoryBuffer implementation [chromium/src : master]
Klaus Weidner (Gerrit)
For context, this is primarily intended to be used for WebVR to reduce the pixel copies needed when transferring images from the GPU process to the native GVR device context.
After discussions with reveman@ and rsesek@, I've attempted to follow the MacOS example of extending SharedMemoryHandle to support an additional native subtype, with an added NO_HANDLE type for invalid handles such as those from the default constructor. A minor wrinkle is that Android does appear to actively transport POSIX SharedMemoryHandles via IPC, so it needs to distinguish both. (Apparently iOS mostly stopped using POSIX shared memory.)
See also billorr@'s https://chromium-review.googlesource.com/c/chromium/src/+/674135 which does something similar for Windows, using its pre-existing SharedMemoryHandle support.
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
Klaus Weidner would like Ken Rockot to review this change.
AHardwareBuffer-based GpuMemoryBuffer implementation
AHardwareBuffer support is included on Android O and up. The NDK
functions are included as of platform level 26, but Chromium currently
doesn't use this as the target level, so we have to use dynamic
loading to get the symbols at runtime.
This is encapsulated in gfx::AndroidHardwareBufferCompat which makes
it a bit easier to work with them across Android versions.
The implementation adds a new ANDROID_HARDWARE_BUFFER subtype to the
SharedMemoryHandle Android implementation in addition to the pre-existing
ashmem support, including IPC transport via a SocketPair. It uses one end
to write the native object using a NDK call, and the other end is a file
descriptor sendable via usual IPC methods from which the receiver can fetch
the object.
BUG=761432
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I9edfa733ffccea21dd58a3940aa96cb6955c8f09
---
M base/BUILD.gn
A base/android/android_hardware_buffer_abi.h
A base/android/android_hardware_buffer_compat.cc
A base/android/android_hardware_buffer_compat.h
M base/memory/shared_memory_handle.h
A base/memory/shared_memory_handle_android.cc
M gpu/BUILD.gn
M gpu/ipc/client/BUILD.gn
M gpu/ipc/client/gpu_memory_buffer_impl.cc
A gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.cc
A gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.h
A gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer_unittest.cc
M gpu/ipc/common/gpu_memory_buffer_support.cc
M gpu/ipc/host/gpu_memory_buffer_support.cc
M gpu/ipc/service/BUILD.gn
M gpu/ipc/service/gpu_memory_buffer_factory.cc
A gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.cc
A gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.h
M ipc/ipc_message_utils.cc
M ui/gfx/gpu_memory_buffer.cc
M ui/gfx/gpu_memory_buffer.h
M ui/gfx/mojo/buffer_types.mojom
M ui/gfx/mojo/buffer_types_struct_traits.cc
M ui/gfx/mojo/buffer_types_struct_traits.h
M ui/gl/gl_image_egl.cc
M ui/gl/gl_image_egl.h
26 files changed, 1,036 insertions(+), 38 deletions(-)
Klaus Weidner (Gerrit)
+rockot for ipc/ipc_message_utils
Klaus Weidner (Gerrit)
FYI, I'm still looking into adding more tests for this, though it's a bit tricky since AFAIK none of the bots are running Android O yet. Since this change touches a fair amount of code including additions to //base, I wanted to make sure that it's on the right track overall to avoid adding tests for code that may turn out to be doing the wrong thing ;-)
Kenneth Russell (Gerrit)
1 comment:
File base/memory/shared_memory_handle_android.cc:
Patch Set #12, Line 102:
write_fd.get());I'm not an expert on Chrome's IPC or Mojo, but rather than creating a new socket pair just to send the AHardwareBuffer to it, and then send the read socket's fd to the other process, is it possible to send the AHardwareBuffer's handle on the socket which Mojo uses?
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
1 comment:
Patch Set #12, Line 102:
write_fd.get());
I'm not an expert on Chrome's IPC or Mojo, but rather than creating a new socket pair just to send t […]
I was thinking about this also, but got lost in IPC internals while looking for a way to do this, and I got the impression it might end up too invasive. dcheng@ or others, please let me know if you think that would be the right way to do it.
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
David Reveman (Gerrit)
9 comments:
AHardwareBuffer support is included on Android O and up. The NDK
functions are included as of platform level 26, but Chromium currently
doesn't use this as the target level, so we have to use dynamic
loading to get the symbols at runtime.
can we just change the target level for chrome? why is dynamic loading not needed just because target level changed? do we build chrome for each target level we support?
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.cc:
Patch Set #13, Line 25:
DVLOG(2) << __FUNCTION__;Please cleanup all this type of debug logging before asking for review.
Patch Set #13, Line 44:
switch (format) {move this into a separate helper function like other GMB impls
Patch Set #13, Line 57:
switch (usage) {move this into a separate helper function like other GMB impls
Patch Set #13, Line 104:
buffer_sizewhat is this used for?
Patch Set #13, Line 108:
CreateFromSpecI think it would be a bit more consistent to have an AHardwareBufferDesc(size, format, usage) function that cannot fail and then call AHardwareBuffer_allocate here.
Patch Set #13, Line 168:
base::Closure GpuMemoryBufferImplAndroidHardwareBuffer::AllocateForTesting(fyi, no need to handle failures in this case. just DCHECK if format or anything else is not supported as we shouldn't be trying to test unsupported configurations.
Patch Set #13, Line 22:
bool InitializeFromHardwareBuffer(const struct AHardwareBuffer* buffer,can you remove this and have the caller use eglGetNativeClientBufferANDROID?
Patch Set #13, Line 40:
void OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd,why add this as part of this patch?
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
9 comments:
AHardwareBuffer support is included on Android O and up. The NDK
functions are included as of platform level 26, but Chromium currently
doesn't use this as the target level, so we have to use dynamic
loading to get the symbols at runtime.
can we just change the target level for chrome? why is dynamic loading not needed just because targe […]
The functions used here were added to the NDK libraries as required Android features at platform level 26, so they would be usable as plain functions without manual dynamic loading if that were used as the NDK platform build level. However, in practice this would require raising the sdkMin version for the entire application to API level 26, and this would make Chrome only show in the Play Store for devices running Android O and newer. That's obviously a nonstarter.
There's a fairly long (though somewhat opinionated) description here: https://stackoverflow.com/a/41079462
There may be ways to do tricky things such as offering multiple APKs on Play Store or shipping multiple .so files in an APK for runtime switching, but that seems unreasonable.
The Java SDK supports separately setting minimum, target, and compile SDK levels, but the NDK does not support this. Using compatible declarations together with manual dynamic loading appears to be the usual approach here. Any changes to the NDK platform level need to be done very carefully due to risk of incompatibility.
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.cc:
Patch Set #13, Line 25:
AHardwareBuffer_Desc desc;
Please cleanup all this type of debug logging before asking for review.
Sorry about that, done. I've left a small amount of error logging, let me know if this seems appropriate now.
Patch Set #13, Line 44:
default:
move this into a separate helper function like other GMB impls
This is a bit different from the other GMB implementations since it needs to convert the Chrome types to AHArdwareBufferDesc equivalents for the supported types. I've moved it to a no-fail "GetBufferConfig" method that's called after checking the already-existing IsConfigurationSupported() logic, with some DCHECK/NOTREACHED as sanity check in case the code gets updated inconsistently.
Patch Set #13, Line 57:
return desc;
move this into a separate helper function like other GMB impls
See previous.
what is this used for?
Removed. I was following the SharedMemory implementation here, it seems to be a sanity check on the size calculation to avoid integer overflow or similar based on safe_math and ValueOrDie, but I'm not sure if it adds much benefit since I'm not using the resulting buffer_size for the allocation.
This does trust the AHardwareBuffer allocation now though to do the right thing, please let me know if you want me to add additional size sanity checks.
I think it would be a bit more consistent to have an AHardwareBufferDesc(size, format, usage) functi […]
Done
fyi, no need to handle failures in this case. […]
Done
Patch Set #13, Line 22:
// Overridden from GLImage:
can you remove this and have the caller use eglGetNativeClientBufferANDROID?
Done. I was assuming that direct GL commands would largely be expected to stay in ui/gl/, but it looks as if direct_composition_surface_win and related code in gpu/ipc/service/ use them fairly extensively already.
why add this as part of this patch?
I'm actually confused about this myself. OnMemoryDump is a pure abstract method in GlImage that didn't get defined or overridden for GlImageEGL (as far as I can tell), so the compiler complained to me that the "new gl: GLImageEGL(size)" call in the factory code was an invalid attempt to construct an abstract class instance. I agree with the compiler, but it's unclear to me how come this never caused issues previously. My guess is that these were only being instantiated via GLImageNativePixmap which does override OnMemoryDump.
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
David Reveman (Gerrit)
21 comments:
AHardwareBuffer support is included on Android O and up. The NDK
functions are included as of platform level 26, but Chromium currently
doesn't use this as the target level, so we have to use dynamic
loading to get the symbols at runtime.
The functions used here were added to the NDK libraries as required Android features at platform lev […]
Ok, then it seems like the right thing to do is always use dynamic loading for now and protect the code that will use this with a:
if (base::android::BuildInfo::GetInstance()->sdk_int() >=
base::android::SDK_VERSION_OREO)check. There's some precedence for this here:
https://cs.chromium.org/chromium/src/net/socket/udp_socket_posix.cc?l=505And I think this check should be done by the GMB code that advertises native support but fine of course to have a helper function such as IsAndroidHardwareBufferSupported in base/ if useful.
File base/android/android_hardware_buffer_abi.h:
Patch Set #14, Line 1:
// Copyright 2017 The Chromium Authors. All rights reserved.should we add hardware_buffer.h to third_party instead of this copy and paste here?
File base/android/android_hardware_buffer_compat.cc:
Patch Set #14, Line 41:
static bool DidCheckAHardwareBufferSupport = false;g_did_check_ahardware_buffer_support, if you keep this but I feel like this is better handled using on demand loading of all symbols when first needed.
Patch Set #14, Line 62:
bool LoadFunctions() {This seems racy. what if LoadFunctions is called from another thread in between DidCheckAHardwareBufferSupport is set to true and HaveAHardwareBufferSupport is assigned a value?
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.h:
Patch Set #14, Line 21:
Android AHardwareBuffernit: "Android hardware buffer" or just "AHardwareBuffer"
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.cc:
Patch Set #14, Line 22:
GetBufferConfigGetBufferDescription?
// On create, all elements must be initialized, including setting the
// "reserved for future use" (rfu) fields to zero.maybe "desc = {}" above instead of setting rfu fields below
Patch Set #14, Line 36:
// color order doesn't match for AHARDWAREBUFFER_FORMAT_R5G6B5_UNORM.is it RGB_565 you want instead? I'd remove this TODO unless it's something you know we need to add in the future.
Patch Set #14, Line 85:
if (!base::AndroidHardwareBufferCompat::IsSupportAvailable())DCHECK? I'd like us to fail much earlier if this is not supported.
Patch Set #14, Line 88:
if (!IsConfigurationSupported(format, usage))remove this as we shouldn't reach this if not supported and you already have DCHECKs in GetBufferConfig.
nit: buffer
Patch Set #14, Line 95:
LOG(ERROR) << __FUNCTION__ << ": Failed to allocate AHardwareBuffer";LOG(ERROR) << "Failed to allocate AHardwareBuffer";
nit: buffer
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer_unittest.cc:
Patch Set #14, Line 19:
TEST(GpuMemoryBufferImplAndroidHardwareBuffer, Create) {we have the same test for shmem. can we add it to the template instead of duplicating code?
File gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.h:
#include <unordered_map>
#include <utility>
#include "base/hash.h"
#include "base/macros.h"
#include "base/synchronization/lock.h"
#include "gpu/command_buffer/service/image_factory.h"
#include "gpu/gpu_export.h"
#include "gpu/ipc/service/gpu_memory_buffer_factory.h"some of these includes are clearly not needed. please only include what you use in this and other files
File gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.cc:
remove blank line here and below. and they should be in alphabetical order
Patch Set #14, Line 53:
// TRACE_EVENT0("gpu", "GpuMemoryBufferFactoryAndroidHardwareBuffer::Create");either have trace event here or remove it completely
Patch Set #14, Line 55:
CHECK_EQDCHECK_EQ
Patch Set #14, Line 58:
if (!buffer)why would this fail? can we DCHECK instead?
Patch Set #14, Line 37:
void OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd,Instead of this, please add a minimal GLImageAndroidHardwareBuffer class and appropriate GLImage unit tests. See unit tests for other GLImage impls.
I'm actually confused about this myself. […]
GLImageEGL is not meant to be a full implementation. I think we should add a GLImageAHardwareBuffer to be consistent with GLImageNativePixmap.
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
There's still a few unresolved comments, main one is that I haven't added gl_image_ahardwarebuffer tests yet.
Apart from the listed comments, I've also made a minor change to a field_trial unittest. My SharedMemoryHandle class has a sanity check that GetHandle requires a valid file descriptor, and the test used -1 for this. I'd prefer to keep the sanity check in place. Let me know if I should pull this out into a separate CL or ditch the sanity check instead.
21 comments:
AHardwareBuffer support is included on Android O and up. The NDK
functions are included as of platform level 26, but Chromium currently
doesn't use this as the target level, so we have to use dynamic
loading to get the symbols at runtime.
Ok, then it seems like the right thing to do is always use dynamic loading for now and protect the c […]
I'm adding this check as a requirement, but I don't think it's safe to treat this as a sufficient test for support being available. I think it's safer to continue handling the case where symbol loading may fail even on a >= Oreo system, to avoid potential crashes if this runs on a weird device that may not entirely follow CDD requirements.
The code already uses this for advertising support, gpu/ipc/common/gpu_memory_buffer_support.cc's IsNativeGpuMemoryBufferConfigurationSupported will return false if base::AndroidHardwareBufferCompat::IsSupportAvailable() is false.
File base/android/android_hardware_buffer_abi.h:
Patch Set #14, Line 1:
// Copyright 2017 The Chromium Authors. All rights reserved.
should we add hardware_buffer. […]
Good question. I'm not sure if it makes sense to separately add it to third_party since it's included in current NDKs, the current copy is just out of date. Should I file a bug for third_party/android_tools/ndk/ to be refreshed so that this can be removed? This could potentially work even without updating the NDK platform level thanks to the new SDK's unified headers where the file would be available as sysroot/usr/include/android/hardware_buffer.h
As far as I know updating the SDK is a somewhat tricky operation due to compatibility concerns even without changing the build level, so I'd prefer to tackle this separately.
File base/android/android_hardware_buffer_compat.cc:
Patch Set #14, Line 41:
// We want to try loading symbols one time only, since retries aren't going tog_did_check_ahardware_buffer_support, if you keep this but I feel like this is better handled using […]
Renamed, using g_ahardwarebuffer_support since ahardware_buffer looked strange.
As far as I know I need both - I don't trust SDK version >= OREO to be a sufficient condition, so I still would prefer to be prepared for the case that symbol loading fails.
I've replaced the pair of booleans with a tristate enum to make this a bit clearer, and to help avoid potential memory inconsistencies due to separate storage.
Patch Set #14, Line 62:
if (!name) { \This seems racy. […]
You're right, I added a lazy initialized lock to protect the state updates.
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.h:
nit: "Android hardware buffer" or just "AHardwareBuffer"
Done
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.cc:
GetBufferDescription?
Done
// "reserved for future use" (rfu) fields to zero.
desc.width = size.width();
maybe "desc = {}" above instead of setting rfu fields below
Done
Patch Set #14, Line 36:
case gfx::BufferFormat::RGBX_8888:is it RGB_565 you want instead? I'd remove this TODO unless it's something you know we need to add i […]
Removed, it's something I'd like to support at some point but this would need spec changes so that a WebGL drawing context can request a low-bit-depth canvas.
Patch Set #14, Line 85:
return nullptr;
DCHECK? I'd like us to fail much earlier if this is not supported.
Done
Patch Set #14, Line 88:
return base::WrapUnique(new GpuMemoryBufferImplAndroidHardwareBuffer(remove this as we shouldn't reach this if not supported and you already have DCHECKs in GetBufferCon […]
Done
nit: buffer
Done
Patch Set #14, Line 95:
emoryBufferImplAndroidHardwareBuffer::Create(
LOG(ERROR) << "Failed to allocate AHardwareBuffer";
we have the same test for shmem. […]
Done, though this is not entirely pretty. The APIs don't quite match, SharedMemory's Create() method and constructor don't take a "usage" parameter. I've added a SharedMemory-compatible Create to the AHardwareBuffer impl to share tests.
File gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.h:
#include "gpu/command_buffer/service/image_factory.h"
#include "gpu/gpu_export.h"
#include "gpu/ipc/service/gpu_memory_buffer_factory.h"
namespace gl {
class GLImage;
}
namespace gpu {some of these includes are clearly not needed. […]
Done. Out of curiosity, is there an automated tool to help do this?
File gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.cc:
Patch Set #14, Line 10:
#include "ui/gl/gl_image_ahardwarebuffer.h"remove blank line here and below. […]
Done
either have trace event here or remove it completely
Done
Patch Set #14, Line 55:
DCHECK(bDCHECK_EQ
Done
why would this fail? can we DCHECK instead?
Replaced with DCHECKs.
Patch Set #14, Line 37:
const gfx::RectF& crop_rect) override;Instead of this, please add a minimal GLImageAndroidHardwareBuffer class and appropriate GLImage uni […]
I've created the new subclass, but I'm still looking into setting up the unit test. For example, the test template needs a CreateSolidColorImage method which is easy for true shared memory, but a bit tricky here for a GPU-only image.
(Leaving unresolved for now.)
Patch Set #13, Line 40:
protected:GLImageEGL is not meant to be a full implementation. […]
Done, and I added a comment that this class is intentionally abstract.
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
David Reveman (Gerrit)
20 comments:
AHardwareBuffer support is included on Android O and up. The NDK
functions are included as of platform level 26, but Chromium currently
doesn't use this as the target level, so we have to use dynamic
loading to get the symbols at runtime.
I'm adding this check as a requirement, but I don't think it's safe to treat this as a sufficient te […]
Devices that doesn't follow CDD requirements doesn't seem like something we should worry about. At least not until we know it exists and we need to support AHardwareBuffer on these devices.
File base/android/android_hardware_buffer_abi.h:
Patch Set #14, Line 1:
// Copyright 2017 The Chromium Authors. All rights reserved.
Good question. […]
Updating third_party/android_tools/ndk/ sounds like something we need to do eventually anyhow so it seems like we should just take care of that now instead of unnecessary temporary copy and paste that will add confusion. I'll leave it up to base/ and third_party/android_tools/ndk/ maintainers to decide this though. Please make sure they are aware of the option.
File base/android/android_hardware_buffer_compat.cc:
Patch Set #14, Line 41:
// We want to try loading symbols one time only, since retries aren't going to
Renamed, using g_ahardwarebuffer_support since ahardware_buffer looked strange. […]
Why is SDK version >= OREO not sufficient? That seems to be sufficient everywhere else in Chrome. Why do we need an exception here?
File base/android/android_hardware_buffer_compat.cc:
Patch Set #16, Line 45:
base::LazyInstanceif you're adding a lazy instance then why not put all function loading logic in the ctor of the instance? that way you can get rid of all this loading state logic and can simply use something like:
g_android_hardware_buffer_support.Get().IsSupported();
g_android_hardware_buffer_support.Get().Allocate();with the lazy instance logic making sure there's no initialization race.
Patch Set #16, Line 50:
PFAHardwareBuffer_allocate AHardwareBuffer_allocate;I'm not a fan of overriding these global symbols as that allows for incorrect usage by trying to use them before LoadFunctions has been called. I would prefer if we used a different set of symbols that can't be accessed without us having a chance load AHardwareBuffer symbols first. E.g. AndroidHardwareBuffer::Allocate, Acquire, etc.
Patch Set #16, Line 60:
#define LOAD_AHARDWAREBUFFER_FUNCTION(name) \A helper function is preferred over a macro. Also, I'd much prefer if we failed hard when a lookup fails. Unless we're aware of a device where this would fail then I don't think we should protect against it.
// cf. base/android/linker/modern_linker_jni.cc
static void* main_dl_handle = nullptr;
if (!main_dl_handle)why is this needed? wouldn't "void* main_dl_handle = dlopen(nullptr, RTLD_NOW)" be the same in this case?
File base/memory/shared_memory_handle.h:
Patch Set #16, Line 156:
NO_HANDLEINVALID or EMPTY? SharedMemoryHandle::NO_HANDLE is a bit weird..
Patch Set #16, Line 158:
POSIXwould ASHMEM be a better description to not have it confused with posix shared memory used on non-Android posix platforms?
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.h:
Patch Set #16, Line 8:
#include "base/memory/shared_memory_handle.h"base/memory/shared_memory.h? as I see usage of that below but not handle..
Patch Set #16, Line 29:
// For compatibility with impl_shared_memory, a Create version thatLEt's add usage argument to shared memory impl instead.
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.cc:
Patch Set #16, Line 63:
buffernit: handle? or buffer_handle?
handle.offset = 0;
handle.stride = stride(0);nit: let's not set these unless we plan on supporting them for import. setting them makes it seem like we support offset and stride..
File gpu/ipc/client/gpu_memory_buffer_impl_create_test_template.h:
Patch Set #16, Line 1:
// Copyright 2015 The Chromium Authors. All rights reserved.please don't add a new template file for this. keep it in existing gpu_memory_buffer_impl_test_template.h file instead.
Patch Set #16, Line 23:
auto destroyer = [](bool* destroyed, const gpu::SyncToken& sync_token) {nit: please keep a BufferDestroyed function as before and maintain blank lines below so it's clear that this is just moving code. we can clean up these tests in a follow up if needed
File gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.h:
#include "gpu/command_buffer/service/image_factory.h"
#include "gpu/gpu_export.h"
#include "gpu/ipc/service/gpu_memory_buffer_factory.h"
namespace gl {
class GLImage;
}
namespace gpu {
Done. […]
Not sure what the current status of clang-include-fixer is. I think it might work if you modify a file but maybe not when adding a new file. Not sure.
File ui/gl/gl_image_ahardwarebuffer.h:
Patch Set #16, Line 20:
bool Initialize(EGLenum target, EGLClientBuffer buffer, const EGLint* attrs);is this used? please remove if not
File ui/gl/gl_image_ahardwarebuffer.cc:
DCHECK(thread_checker_.CalledOnValidThread());
if (egl_image_ != EGL_NO_IMAGE_KHR) {
EGLBoolean result =
eglDestroyImageKHR(GLSurfaceEGL::GetHardwareDisplay(), egl_image_);
if (result == EGL_FALSE) {
DLOG(ERROR) << "Error destroying EGLImage: "
<< ui::GetLastEGLErrorString();
}
}this should be done by GLImageEGL dtor and not here.
// This is an abstract base class, must define OnMemoryDump in a subclass.
//nit: please move this comment above l.16.
Patch Set #14, Line 37:
const gfx::RectF& crop_rect) override;
I've created the new subclass, but I'm still looking into setting up the unit test. […]
Can't we use AHardwareBuffer_lock/unlock for this? No need to expose lock/unlock in the GMB API but using it for tests so we can at least run this on a device where we know it's supposed to work to verify the logic would be nice.
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
I'm still working on the GLImageAHardwareBuffer test, but wanted to upload a patch that addresses most of the outstanding comments and to keep in sync with the split-out patches. I'll follow up once the image test is done.
20 comments:
AHardwareBuffer support is included on Android O and up. The NDK
functions are included as of platform level 26, but Chromium currently
doesn't use this as the target level, so we have to use dynamic
loading to get the symbols at runtime.
Devices that doesn't follow CDD requirements doesn't seem like something we should worry about. […]
I agree, the one concern I still have is that the AHardwareBuffer GMB code gets activated even in cases when it's not really needed due to being part of the generic GMB handle struct, and I'd want to avoid potential crashes as part of unrelated code doing checks as a side effect. This should be doable as part of the lazy initialization.
For example, I had a mistake in earlier versions of the code that didn't correctly handle null AHardwareBuffer GMB handles, and this unexpectedly broke the Google Search and Docs web pages.
File base/android/android_hardware_buffer_abi.h:
Patch Set #14, Line 1:
// Copyright 2017 The Chromium Authors. All rights reserved.
Updating third_party/android_tools/ndk/ sounds like something we need to do eventually anyhow so it […]
After discussing options with agrieve@ and jbudorick@, I added a "me too" to the existing http://crbug.com/771171 that requests an NDK update, and added a reference to that in a TODO to delete this file once the NDK is updated.
File base/android/android_hardware_buffer_compat.cc:
Patch Set #14, Line 41:
dlsym(main_dl_handle, "AHardwareBuffer_acquire");Why is SDK version >= OREO not sufficient? That seems to be sufficient everywhere else in Chrome. […]
You've convinced me that it's overly paranoid, removing this extra level of checking.
File base/android/android_hardware_buffer_compat.cc:
Patch Set #16, Line 45:
lsym(main_dl_handlif you're adding a lazy instance then why not put all function loading logic in the ctor of the inst […]
Done, with a few differences. I'm keeping IsSupportAvailable as a static method to ensure that the instance only gets constructed if it's actually needed, and using a GetInstance class method instead of exposing the lazy instance directly as a global. How does this look?
Patch Set #16, Line 50:
DCHECK(send_handle_);I'm not a fan of overriding these global symbols as that allows for incorrect usage by trying to use […]
Done.
A helper function is preferred over a macro. […]
I've removed the macro and inlined the dlsym calls. A helper method isn't really useful here, I've added DCHECKs for loading symbols.
allocate_(desc, out_buffer);
}why is this needed? wouldn't "void* main_dl_handle = dlopen(nullptr, RTLD_NOW)" be the same in this […]
Done. It's equivalent when this only gets called once, especially with the new constructor which makes it clearer.
File base/memory/shared_memory_handle.h:
Patch Set #16, Line 156:
INVALID,
INVALID or EMPTY? SharedMemoryHandle::NO_HANDLE is a bit weird..
Done (using INVALID)
Patch Set #16, Line 158:
ASHMEwould ASHMEM be a better description to not have it confused with posix shared memory used on non-An […]
Done
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.h:
Patch Set #16, Line 8:
#include "base/memory/shared_memory.h"
base/memory/shared_memory.h? as I see usage of that below but not handle..
Done
Patch Set #16, Line 29:
static std::unique_ptr<GpuMemoryBufferImplAndroidHardwareBuffer>
LEt's add usage argument to shared memory impl instead.
Done, split out into separate crrev.com/c/747601 .
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.cc:
nit: handle? or buffer_handle?
Done
gfx::BufferUsage usage,
gfx::GpuMemoryBufferHandnit: let's not set these unless we plan on supporting them for import. […]
Done
File gpu/ipc/client/gpu_memory_buffer_impl_create_test_template.h:
please don't add a new template file for this. […]
Done
nit: please keep a BufferDestroyed function as before and maintain blank lines below so it's clear t […]
I'm not sure if there's a good way to keep it as a function. A plain function fails due to multiple definitions as this gets included multiple times as a header file. I think a static function would fail with "unused function" compiler errors for platforms that don't instantiate the Create test case. (This would be possible if I would keep it in a separate test template file.) I also tried a static member function for the class, but that ended up making even more changes to the test than the lambda did even before trying to get a function pointer to the templated static class member.
I've moved this into the separate change http://crrev.com/c/747604 to keep things cleaner. Since that now only contains this one change, I hope it's now clear enough that it's basically just a code move. Marking as resolved here, let's follow up in that CL?
File gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.h:
#include "gpu/command_buffer/service/image_factory.h"
#include "gpu/gpu_export.h"
#include "gpu/ipc/service/gpu_memory_buffer_factory.h"
namespace gl {
class GLImage;
}
namespace gpu {
Not sure what the current status of clang-include-fixer is. […]
Ack
File ui/gl/gl_image_ahardwarebuffer.h:
Patch Set #16, Line 20:
// Overridden from GLImage:
is this used? please remove if not
Removed, the base class implementation is all that's needed.
File ui/gl/gl_image_ahardwarebuffer.cc:
unsigned GLImageAHardwareBuffer::GetInternalFormat() {
return GL_RGBA;
}
bool GLImageAHardwareBuffer::CopyTexImage(unsigned target) {
return false;
}
this should be done by GLImageEGL dtor and not here.
Done, replaced with relying on the base destructor.
nit: please move this comment above l.16.
Done
Can't we use AHardwareBuffer_lock/unlock for this? No need to expose lock/unlock in the GMB API but […]
Yes, that's what I'm working on for the test. It's not done yet, leaving this item unresolved for now.
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
I've added gl_image_ahardwarebuffer_unittest, though the current implementation would fail on pre-O test systems. What's the best way to make this runtime conditional? billorr's https://chromium-review.googlesource.com/c/chromium/src/+/674135/24/gpu/ipc/service/gpu_memory_buffer_factory_dxgi_unittest.cc used a DISABLED_ prefix, but I couldn't figure out in which circumstances this would get enabled.
1 comment:
Yes, that's what I'm working on for the test. […]
Unit test added, though I'm not sure how to make this conditional on runtime support short of annoying conditionals in the template.
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Kenneth Russell (Gerrit)
I defer review to reveman@; I barely know any of the affected code. Please tell me if you need my review.
Klaus Weidner (Gerrit)
Patch Set 18:
(1 comment)
I've added gl_image_ahardwarebuffer_unittest, though the current implementation would fail on pre-O test systems. What's the best way to make this runtime conditional? billorr's https://chromium-review.googlesource.com/c/chromium/src/+/674135/24/gpu/ipc/service/gpu_memory_buffer_factory_dxgi_unittest.cc used a DISABLED_ prefix, but I couldn't figure out in which circumstances this would get enabled.
I found the right documentation, I default-disabled the tests and added a comment pointing to the --gtest-also-run-disabled-tests flag as a reminder for future me.
David Reveman (Gerrit)
LGTM with nits. Please have base::SharedMemory changes reviewed by base owner that is more familiar with that code before you land this.
Patch set 19:Code-Review +1
13 comments:
File base/android/android_hardware_buffer_compat.cc:
Patch Set #19, Line 5:
#define DCHECK_ALWAYS_ON 1nit: I assume this is only for testing? please remove this before you land the change
File base/memory/shared_memory_handle_android.cc:
Patch Set #19, Line 20:
const base::FileDescriptor& file_descriptor,nit: not need for base:: prefix here and below but maybe better to keep it for consistency if this is the common practice in shared memory code today
File ui/gl/gl_image_ahardwarebuffer.h:
nit: not needed in new files
Patch Set #19, Line 9:
#include "base/threading/thread_checker.h"nit: unused?
Patch Set #19, Line 10:
#include "ui/gl/gl_bindings.h"nit: unused?
File ui/gl/gl_image_ahardwarebuffer.cc:
nit: not needed in new files
#include "ui/gl/egl_util.h"
#include "ui/gl/gl_surface_egl.h"nit: are these needed? just move gl_bindings.h here for GL_RGBA below instead?
File ui/gl/gl_image_ahardwarebuffer_unittest.cc:
Patch Set #19, Line 24:
if (!base::AndroidHardwareBufferCompat::IsSupportAvailable()) {nit: ASSERT_TRUE(base::AndroidHardwareBufferCompat::IsSupportAvailable()) as that makes it more clear what is wrong if someone runs this on a device where it's not supported.
Patch Set #19, Line 33:
pixel_bytesnit: kBytesPerPixel
nit: result?
Patch Set #19, Line 52:
ret = base::AndroidHardwareBufferCompat::GetInstance().Lock(nit: int result = base::AndroidH...
Patch Set #19, Line 54:
(void**)(&data)nit: static_cast<void**>(&data)? or reinterpret_cast if needed..
Patch Set #19, Line 81:
colornit: kColor or maybe kTransparentBlack
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
Patch Set 19: Code-Review+1
(13 comments)
LGTM with nits. Please have base::SharedMemory changes reviewed by base owner that is more familiar with that code before you land this.
13 comments:
File base/android/android_hardware_buffer_compat.cc:
Patch Set #19, Line 5:
#include "base/android/android_hardware_buffer_compat.h"
nit: I assume this is only for testing? please remove this before you land the change
Patch Set #19, Line 20:
const base::FileDescriptor& file_descriptor,
nit: not need for base:: prefix here and below but maybe better to keep it for consistency if this i […]
It looks as if function arguments consistently use base:: while function bodies don't. I've updated this file to match.
nit: not needed in new files
Done
Patch Set #19, Line 9:
#include "ui/gl/gl_export.h"nit: unused?
Done
Patch Set #19, Line 10:
#include "ui/gl/gl_image_egl.h"nit: unused?
Done
nit: not needed in new files
Done
#include "ui/gl/gl_bindings.h"
nit: are these needed? just move gl_bindings. […]
Done
File ui/gl/gl_image_ahardwarebuffer_unittest.cc:
Patch Set #19, Line 24:
CHECK(base::AndroidHardwareBufferCompat::IsSupportAvailable());nit: ASSERT_TRUE(base::AndroidHardwareBufferCompat::IsSupportAvailable()) as that makes it more clea […]
Done, using CHECK since ASSERT_TRUE only works in void-return functions.
Patch Set #19, Line 33:
_memory_bufnit: kBytesPerPixel
Done
nit: result?
Done
nit: int result = base::AndroidH...
Done, using lock_result and unlock_result (below) instead of reusing the same name.
Patch Set #19, Line 54:
dwareBuffer* bu
nit: static_cast<void**>(&data)? or reinterpret_cast if needed..
Done (needs reinterpret_cast)
nit: kColor or maybe kTransparentBlack
Done
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
rockot@: please review ipc/ipc_message_utils.cc
kbr@: please review gpu/*/BUILD.gn and ui/gl/BUILD.gn
mark@chromium: please review the base/ changes including shared_memory
dcheng@: please review the IPC changes including ui/gfx/mojo/ (or shared memory also if you prefer? This overlaps with mark@chromium)
Thanks in advance, and sorry about the noisy review so far.
Ken Rockot (Gerrit)
ipc LGTM
Patch set 21:Code-Review +1
Kenneth Russell (Gerrit)
various BUILD.gn files LGTM
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
5 comments:
File base/memory/shared_memory_handle.h:
Patch Set #22, Line 177:
!defined(OS_WIN)!defined(OS_WIN) is redundant, since OS_WIN is never OS_POSIX.
Patch Set #22, Line 178:
components“components” sounds funny. Can’t you just call this a SharedMemoryHandle backed by a FileDescriptor?
Patch Set #22, Line 181:
UnfortunatelyIs this advocating for or against a particular usage pattern? I can’t tell why this comment is here. This comment should tell me how to use the interface. Unless it’s very relevant, it shouldn’t opine on abuses of an interface, although it could say something like “this common pattern is bad, do that other thing instead.”
Patch Set #22, Line 191:
has no immediate consequenceThis isn’t an interface contract. The caller is supposed to pass the correct size, period. If they don’t, all bets are off. I don’t think you need to say anything about “wrong |size|” at all.
Patch Set #22, Line 224:
union {I’m a little fearful of this union. Some innocent person might want to make FileDescriptor non-POD.
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
Please let me know if you want me to do more editing on the moved comment.
5 comments:
!defined(OS_WIN) is redundant, since OS_WIN is never OS_POSIX.
Removed.
Patch Set #22, Line 178:
leDescript“components” sounds funny. […]
Done. I didn't write this comment, it's just moved (from line 85 left side) as part of an attempt to unify the conditional sections.
Patch Set #22, Line 181:
mon for existIs this advocating for or against a particular usage pattern? I can’t tell why this comment is here. […]
See previous, I didn't write this comment. I'm interpreting it as a warning to not expect shared memory handles to work right when not following the usual "ok to copy, but only consume from it once" pattern, but I'm not entirely sure.
Patch Set #22, Line 191:
aredMemoryHandle at a laterThis isn’t an interface contract. The caller is supposed to pass the correct size, period. […]
See previous - should I remove it?
Patch Set #22, Line 224:
struct AHardwareBuffer* memory_object_ = nullptr;
I’m a little fearful of this union. Some innocent person might want to make FileDescriptor non-POD.
Flattened into plain members. I was following the MacOS example, but I'm not sure I like it myself, and I doubt they are common enough for the memory savings to be worthwhile.
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
I’d prefer to see the comments improved as long as you’re in here moving them around, but LGTM either way.
Patch set 23:Code-Review +1
2 comments:
File base/android/android_hardware_buffer_abi.h:
Patch Set #23, Line 25:
extern "C" {It doesn’t look like this affects anything here (but it’s also not actively harmful).
File base/memory/shared_memory_handle.h:
Patch Set #23, Line 224:
struct AHardwareBuffer* memory_object_ = nullptr;In C++, you can just write AHardwareBuffer*, no extra struct needed.
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
dcheng@, PTAL?
2 comments:
File base/android/android_hardware_buffer_abi.h:
Patch Set #23, Line 25:
// that forward declarations elsewhere should use "extern "C" to avoid
It doesn’t look like this affects anything here (but it’s also not actively harmful).
I've added a comment. Removing it works also, but then it looks a bit strange without a C++ namespace, and using a namespace would be incompatible with the original header file which we're planning to switch to once the third_party NDK is updated. Currently, forward declarations elsewhere use "extern "C" typedef struct AHardwareBuffer AHardwareBuffer" which works with the original android/hardware_buffer.h.
(This file used to contain function declarations such as AHardwareBuffer_allocate where the "C" linkage would be important, but I switched to using member functions instead.)
File base/memory/shared_memory_handle.h:
Patch Set #23, Line 224:
AHardwareBuffer* memory_object_ = nullptr;
In C++, you can just write AHardwareBuffer*, no extra struct needed.
Removed the "struct" here and elsewhere.
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Sorry for the review delay. Looks pretty good overall, just a few small nits.
10 comments:
Patch Set #24, Line 1127:
if (!is_mac && !is_android && is_posix) {Nit: I would personally find this condition clearer if is_posix was first.
File base/memory/shared_memory_handle.h:
Patch Set #24, Line 173:
Type type);Nit: I would put the Type first (since it disambiguates the FileDescriptor argument)
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.h:
Patch Set #24, Line 39:
static base::Closure AllocateForTesting(const gfx::Size& size,Is this called by something?
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.cc:
Patch Set #24, Line 84:
LOG(ERROR) << "Failed to allocate AHardwareBuffer";Does DLOG instead of LOG work here?
File gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.cc:
Any reason for LOG instead of DLOG?
File ipc/ipc_message_utils.cc:
Patch Set #24, Line 595:
WriteParam(m, static_cast<uint32_t>(p.GetType()));Please add an IPC_ENUM_TRAITS_MAX_VALUE() for this rather than casting to/from integer types.
File ui/gfx/mojo/buffer_types.mojom:
Patch Set #24, Line 53:
LAST = ANDROID_HARDWARE_BUFFERLet's just get rid of the LAST value (it's not needed in mojo).
The EnumTraits that returns this can just return EMPTY_BUFFER or something.
File ui/gl/gl_image_ahardwarebuffer_unittest.cc:
Patch Set #24, Line 22:
scoped_refptr<GLImage> CreateSolidColorImage(const gfx::Size& size,Nit: Include base/memory/scoped_refptr.h
Patch Set #24, Line 23:
const uint8_t color[4]) const {Nit: by ref, though the syntax looks kind of funny: const uint8_t (&color)[4]
Patch Set #24, Line 77:
scoped_refptr<GLImageAHardwareBuffer> image(Nit: consider writing:
auto image = base::MakeRefCounted<GLImageAHardwareBuffer>(size);
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
10 comments:
Patch Set #24, Line 1127:
if (is_posix && !is_mac && !is_android) {
Nit: I would personally find this condition clearer if is_posix was first.
Done
File base/memory/shared_memory_handle.h:
Patch Set #24, Line 173:
const base::FileDescriptor& file_descriptor,
Nit: I would put the Type first (since it disambiguates the FileDescriptor argument)
Patch Set #24, Line 39:
static base::Closure AllocateForTesting(const gfx::Size& size,Is this called by something?
Yes, the call is in the pre-existing gpu_memory_buffer_impl_test_template.h as instantiated from gpu_memory_buffer_impl_android_hardware_buffer_unittest.cc . (I made changes in http://crrev.com/c/747604 and http://crrev.com/c/747601 to share that test.)
File gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.cc:
Patch Set #24, Line 84:
LOG(ERROR) << "Failed to allocate AHardwareBuffer";Does DLOG instead of LOG work here?
I'd prefer to keep this error visible in production builds since buffer allocation is something that could sometimes fail at runtime, i.e. due to lack of memory, and this message would be helpful in figuring out what's going on, especially since error handling in consumers of this may no longer refer to AHardwareBuffer specifically.
Is there documentation about logging best practices? My interpretation is that "shouldn't happen" logic errors should be DLOG while rare-but-possible runtime errors would be a candidate for keeping, but existing usage seems to vary.
File gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.cc:
Any reason for LOG instead of DLOG?
Changed to DLOG - this shouldn't fail on systems that support AHardwareBuffer.
File ipc/ipc_message_utils.cc:
Patch Set #24, Line 595:
#endif
Please add an IPC_ENUM_TRAITS_MAX_VALUE() for this rather than casting to/from integer types.
Done, via manual ParamTraits override as discussed in IM.
File ui/gfx/mojo/buffer_types.mojom:
Let's just get rid of the LAST value (it's not needed in mojo). […]
Done
File ui/gl/gl_image_ahardwarebuffer_unittest.cc:
Patch Set #24, Line 22:
public:Nit: Include base/memory/scoped_refptr. […]
Done
Patch Set #24, Line 23:
scoped_refptr<GLImage> CreateSolidColorImage(const gfx::Size& size,
Nit: by ref, though the syntax looks kind of funny: const uint8_t (&color)[4]
That's incompatible with existing usage in the template:
../../ui/gl/test/gl_image_test_template.h:314:57: error: reference to type 'const uint8_t [4]' could not bind to an lvalue of type 'const uint8_t *' (aka 'const unsigned char *')
this->delegate_.CreateSolidColorImage(image_size, image_color);Let me know if you feel strongly about changing it, this would require modifying the template and other tests.
Nit: consider writing: […]
Done
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner
Klaus Weidner (Gerrit)
I still need to update the Read usage side for the new specialization, I'll do that next.
Daniel Cheng (Gerrit)
Patch set 26:Code-Review +1
1 comment:
File ui/gl/gl_image_ahardwarebuffer_unittest.cc:
Patch Set #24, Line 23:
scoped_refptr<GLImage> CreateSolidColorImage(const gfx::Size& size,That's incompatible with existing usage in the template: […]
Ah I see. Maybe in a followup, certainly I wouldn't expect an arbitrary uint8_t pointer to point to exactly 4 valid bytes =)
To view, visit change 680100. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
Patch set 27:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fix android subtype read" https://chromium-review.googlesource.com/c/680100/27
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/680100/27
Bot data: {"action": "start", "triggered_at": "2017-11-02T21:10:01.0Z", "cq_cfg_revision": "31ea7a9a715f873162e3599a52d1ba915e17d54a", "revision": "fe2fcb4efc3224c38783478dd4a0e0f780596744"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
win7_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win7_chromium_rel_ng/builds/33907)
Klaus Weidner (Gerrit)
Patch set 27:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fix android subtype read" https://chromium-review.googlesource.com/c/680100/27
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/680100/27
Bot data: {"action": "start", "triggered_at": "2017-11-03T03:46:55.0Z", "cq_cfg_revision": "31ea7a9a715f873162e3599a52d1ba915e17d54a", "revision": "fe2fcb4efc3224c38783478dd4a0e0f780596744"}
Commit Bot (Gerrit)
Commit Bot merged this change.
AHardwareBuffer-based GpuMemoryBuffer implementation
AHardwareBuffer support is included on Android O and up. The NDK
functions are included as of platform level 26, but Chromium currently
doesn't use this as the target level, so we have to use dynamic
loading to get the symbols at runtime.
This is encapsulated in gfx::AndroidHardwareBufferCompat which makes
it a bit easier to work with them across Android versions.
The implementation adds a new ANDROID_HARDWARE_BUFFER subtype to the
SharedMemoryHandle Android implementation in addition to the pre-existing
ashmem support, including IPC transport via a SocketPair. It uses one end
to write the native object using a NDK call, and the other end is a file
descriptor sendable via usual IPC methods from which the receiver can fetch
the object.
BUG=761432
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I9edfa733ffccea21dd58a3940aa96cb6955c8f09
Reviewed-on: https://chromium-review.googlesource.com/680100
Commit-Queue: Klaus Weidner <kla...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Mark Mentovai <ma...@chromium.org>
Reviewed-by: Ken Rockot <roc...@chromium.org>
Reviewed-by: Kenneth Russell <k...@chromium.org>
Reviewed-by: David Reveman <rev...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513710}
---
M base/BUILD.gn
A base/android/android_hardware_buffer_abi.h
A base/android/android_hardware_buffer_compat.cc
A base/android/android_hardware_buffer_compat.h
M base/android/build_info.h
M base/memory/shared_memory_handle.h
A base/memory/shared_memory_handle_android.cc
M gpu/BUILD.gn
M gpu/ipc/client/BUILD.gn
M gpu/ipc/client/gpu_memory_buffer_impl.cc
A gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.cc
A gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer.h
A gpu/ipc/client/gpu_memory_buffer_impl_android_hardware_buffer_unittest.cc
M gpu/ipc/common/gpu_memory_buffer_support.cc
M gpu/ipc/host/gpu_memory_buffer_support.cc
M gpu/ipc/service/BUILD.gn
M gpu/ipc/service/gpu_memory_buffer_factory.cc
A gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.cc
A gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.h
M ipc/ipc_message_utils.cc
M ipc/ipc_message_utils.h
M ui/gfx/gpu_memory_buffer.cc
M ui/gfx/gpu_memory_buffer.h
M ui/gfx/mojo/buffer_types.mojom
M ui/gfx/mojo/buffer_types_struct_traits.cc
M ui/gfx/mojo/buffer_types_struct_traits.h
M ui/gl/BUILD.gn
A ui/gl/gl_image_ahardwarebuffer.cc
A ui/gl/gl_image_ahardwarebuffer.h
A ui/gl/gl_image_ahardwarebuffer_unittest.cc
M ui/gl/gl_image_egl.h
31 files changed, 1,272 insertions(+), 42 deletions(-)