Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2285593002:
Add ScopedSurfaceRequestManager (Closed)
|
Created:
4 years, 3 months ago by tguilbert Modified:
4 years, 3 months ago Reviewers:
David Trainor- moved to gerrit, watk, ncarter (slow), dcheng, liberato (no reviews please), no sievers, DaleCurtis CC:
chromium-reviews, darin-cc_chromium.org, jam, piman+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ScopedSurfaceRequestManager
WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and
the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU
process to a MediaPlayer in the BrowserProcess.
MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a
way to receive surface textures from the GPU. This change introduces the
ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit.
The Manager allows the registration of a callback in the Browser
process, which can receive a ScopedJavaSurface. Upon registration,
the manager creates a token that can be used at a later time to
fulfill the request.
The Conduit forwards surface texture to the Manager, using the
issued token to route the ScopedJavaSurface to the right request.
From the GPU process, surfaces are sent via the ChildProcessServices
and the IChildProcessCallback, the same way that EstablishSurfacePeer
currently does. In single process mode, the Manager directly implements
the Conduit interface.
Security recommended 128 bits as the token size. It is represented by
base::Nonce class, which supports serialization accross IPC and Mojo,
and is easy to send as two uint64_t through JNI and AIDL.
The CL that covers the integration within StreamTexture and
MediaPlayerRenderer is tracked by the same bug, and can be
found at https://codereview.chromium.org/2282633002/.
Adding palmer@ as TBR, since he is the only owner for aidl files, but
is unavailable ATM.
TBR=palmer
BUG=627658
TEST=Added UTs, manually verified that it worked in the prototype.
CQ_INCLUDE_TRYBOTS=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
Committed: https://crrev.com/f08485bfd9514a905dda4ca23eca02e4f1f3d72c
Cr-Commit-Position: refs/heads/master@{#419971}
Patch Set 1 #Patch Set 2 : Fixing typos #Patch Set 3 : Changing |request_id| type from int to uint64_t #Patch Set 4 : Fixed signed/unsigned in UTs #
Total comments: 13
Patch Set 5 : Simplified manager interface. Renamed to Conduit. #
Total comments: 22
Patch Set 6 : Addressing comments #
Total comments: 24
Patch Set 7 : Addressed comments. #
Total comments: 2
Patch Set 8 : Fixed DCHECKS and rebased #Patch Set 9 : Added 128 bit MediaToken #Patch Set 10 : rebase #Patch Set 11 : Remove media token. Switch to base64 string #Patch Set 12 : Fixing comments. #
Total comments: 2
Patch Set 13 : Changed to base::Nonce #Patch Set 14 : Changed JNI serialization to 2 longs #
Total comments: 11
Patch Set 15 : Update to UnguessableToken. Addressed comments. #
Total comments: 11
Patch Set 16 : Addressed latest comments. #Patch Set 17 : Remove unused string headers. #Patch Set 18 : Rebase. #Patch Set 19 : Added RunUntilIdle() to 2 UTs #Dependent Patchsets: Messages
Total messages: 111 (76 generated)
Description was changed from
==========
Add ScopedSurfaceRequestNanager
WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and
the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU
process to a MediaPlayer in the BrowserProcess.
MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a
way to receive surface textures from the GPU.
This change introduces the ScopedSurfaceRequestManager interface, and
concrete implementations to be used from the GPU and Browser processes.
It allows the registration of a callback in the Browser process, which
can then receive a ScopedJavaSurface from the GPU process
(or from the browser process, in the case of the single-process).
In the case of the SurfaceTexture being forwarded by the GPU process,
the order in which one should read the functions to follow the
execution flow is as follows:
ScopedSurfaceRequestManager.h --FulfillScopedSurfaceRequest()
ChildProcesServiceImpl.cc -- FulfillScopedSurfaceRequest()
<JNI switch from native to Java>
ChildProcessServiceImpl.Java -- fulfillScopedSurfaceRequest()
IChildProcessCallback.aidl -- fulfillScopedSurfaceRequest()
<Android Binder from the GPU to the browser process>
ChildProcessLauncher.java -- fulfillScopedSurfaceRequest()
<JNI switch from Java to native>
ChilProcessLauncherAndroid.cc -- CompleteScopedSurfaceRequest()
The CL that covers the integration within StreamTexture and
MediaPlayerRenderer will soon follow, and will be tracked by the
same bug.
BUG=627658
TEST=Added UTs, manually verified that it worked in the prototype.
==========
to
==========
Add ScopedSurfaceRequestNanager
WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and
the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU
process to a MediaPlayer in the BrowserProcess.
MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a
way to receive surface textures from the GPU.
This change introduces the ScopedSurfaceRequestManager interface, and
concrete implementations to be used from the GPU and Browser processes.
It allows the registration of a callback in the Browser process, which
can then receive a ScopedJavaSurface from the GPU process
(or from the browser process, in the case of the single-process).
In the case of the SurfaceTexture being forwarded by the GPU process,
the order in which one should read the functions to follow the
execution flow is as follows:
ScopedSurfaceRequestManager.h --FulfillScopedSurfaceRequest()
ChildProcesServiceImpl.cc -- FulfillScopedSurfaceRequest()
<JNI switch from native to Java>
ChildProcessServiceImpl.Java -- fulfillScopedSurfaceRequest()
IChildProcessCallback.aidl -- fulfillScopedSurfaceRequest()
<Android Binder from the GPU to the browser process>
ChildProcessLauncher.java -- fulfillScopedSurfaceRequest()
<JNI switch from Java to native>
ChilProcessLauncherAndroid.cc -- CompleteScopedSurfaceRequest()
The CL that covers the integration within StreamTexture and
MediaPlayerRenderer will soon follow, and will be tracked by the
same bug.
BUG=627658
TEST=Added UTs, manually verified that it worked in the prototype.
CQ_INCLUDE_TRYBOTS=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
==========
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tguilbert@chromium.org changed reviewers: + dalecurtis@google.com, watk@chromium.org
PTAL! The CL that shows how all of this code is integrated in the MediaPlayerRenderer scenario will shortly follow (and I will update the description). I will add owners & security owners once I have media signoff. Thanks! Thomas
Description was changed from
==========
Add ScopedSurfaceRequestNanager
WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and
the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU
process to a MediaPlayer in the BrowserProcess.
MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a
way to receive surface textures from the GPU.
This change introduces the ScopedSurfaceRequestManager interface, and
concrete implementations to be used from the GPU and Browser processes.
It allows the registration of a callback in the Browser process, which
can then receive a ScopedJavaSurface from the GPU process
(or from the browser process, in the case of the single-process).
In the case of the SurfaceTexture being forwarded by the GPU process,
the order in which one should read the functions to follow the
execution flow is as follows:
ScopedSurfaceRequestManager.h --FulfillScopedSurfaceRequest()
ChildProcesServiceImpl.cc -- FulfillScopedSurfaceRequest()
<JNI switch from native to Java>
ChildProcessServiceImpl.Java -- fulfillScopedSurfaceRequest()
IChildProcessCallback.aidl -- fulfillScopedSurfaceRequest()
<Android Binder from the GPU to the browser process>
ChildProcessLauncher.java -- fulfillScopedSurfaceRequest()
<JNI switch from Java to native>
ChilProcessLauncherAndroid.cc -- CompleteScopedSurfaceRequest()
The CL that covers the integration within StreamTexture and
MediaPlayerRenderer will soon follow, and will be tracked by the
same bug.
BUG=627658
TEST=Added UTs, manually verified that it worked in the prototype.
CQ_INCLUDE_TRYBOTS=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
==========
to
==========
Add ScopedSurfaceRequestManager
WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and
the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU
process to a MediaPlayer in the BrowserProcess.
MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a
way to receive surface textures from the GPU.
This change introduces the ScopedSurfaceRequestManager interface, and
concrete implementations to be used from the GPU and Browser processes.
It allows the registration of a callback in the Browser process, which
can then receive a ScopedJavaSurface from the GPU process
(or from the browser process, in the case of the single-process).
In the case of the SurfaceTexture being forwarded by the GPU process,
the order in which one should read the functions to follow the
execution flow is as follows:
ScopedSurfaceRequestManager.h --FulfillScopedSurfaceRequest()
ChildProcesServiceImpl.cc -- FulfillScopedSurfaceRequest()
<JNI switch from native to Java>
ChildProcessServiceImpl.Java -- fulfillScopedSurfaceRequest()
IChildProcessCallback.aidl -- fulfillScopedSurfaceRequest()
<Android Binder from the GPU to the browser process>
ChildProcessLauncher.java -- fulfillScopedSurfaceRequest()
<JNI switch from Java to native>
ChilProcessLauncherAndroid.cc -- CompleteScopedSurfaceRequest()
The CL that covers the integration within StreamTexture and
MediaPlayerRenderer will soon follow, and will be tracked by the
same bug.
BUG=627658
TEST=Added UTs, manually verified that it worked in the prototype.
CQ_INCLUDE_TRYBOTS=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
==========
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Add ScopedSurfaceRequestManager
WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and
the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU
process to a MediaPlayer in the BrowserProcess.
MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a
way to receive surface textures from the GPU.
This change introduces the ScopedSurfaceRequestManager interface, and
concrete implementations to be used from the GPU and Browser processes.
It allows the registration of a callback in the Browser process, which
can then receive a ScopedJavaSurface from the GPU process
(or from the browser process, in the case of the single-process).
In the case of the SurfaceTexture being forwarded by the GPU process,
the order in which one should read the functions to follow the
execution flow is as follows:
ScopedSurfaceRequestManager.h --FulfillScopedSurfaceRequest()
ChildProcesServiceImpl.cc -- FulfillScopedSurfaceRequest()
<JNI switch from native to Java>
ChildProcessServiceImpl.Java -- fulfillScopedSurfaceRequest()
IChildProcessCallback.aidl -- fulfillScopedSurfaceRequest()
<Android Binder from the GPU to the browser process>
ChildProcessLauncher.java -- fulfillScopedSurfaceRequest()
<JNI switch from Java to native>
ChilProcessLauncherAndroid.cc -- CompleteScopedSurfaceRequest()
The CL that covers the integration within StreamTexture and
MediaPlayerRenderer will soon follow, and will be tracked by the
same bug.
BUG=627658
TEST=Added UTs, manually verified that it worked in the prototype.
CQ_INCLUDE_TRYBOTS=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
==========
to
==========
Add ScopedSurfaceRequestManager
WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and
the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU
process to a MediaPlayer in the BrowserProcess.
MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a
way to receive surface textures from the GPU.
This change introduces the ScopedSurfaceRequestManager interface, and
concrete implementations to be used from the GPU and Browser processes.
It allows the registration of a callback in the Browser process, which
can then receive a ScopedJavaSurface from the GPU process
(or from the browser process, in the case of the single-process).
In the case of the SurfaceTexture being forwarded by the GPU process,
the order in which one should read the functions to follow the
execution flow is as follows:
ScopedSurfaceRequestManager.h --FulfillScopedSurfaceRequest()
ChildProcesServiceImpl.cc -- FulfillScopedSurfaceRequest()
<JNI switch from native to Java>
ChildProcessServiceImpl.Java -- fulfillScopedSurfaceRequest()
IChildProcessCallback.aidl -- fulfillScopedSurfaceRequest()
<Android Binder from the GPU to the browser process>
ChildProcessLauncher.java -- fulfillScopedSurfaceRequest()
<JNI switch from Java to native>
ChilProcessLauncherAndroid.cc -- CompleteScopedSurfaceRequest()
The CL that covers the integration within StreamTexture and
MediaPlayerRenderer is tracked by the same bug, and can be found at
https://codereview.chromium.org/2282633002/.
BUG=627658
TEST=Added UTs, manually verified that it worked in the prototype.
CQ_INCLUDE_TRYBOTS=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
==========
Description was changed from
==========
Add ScopedSurfaceRequestManager
WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and
the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU
process to a MediaPlayer in the BrowserProcess.
MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a
way to receive surface textures from the GPU.
This change introduces the ScopedSurfaceRequestManager interface, and
concrete implementations to be used from the GPU and Browser processes.
It allows the registration of a callback in the Browser process, which
can then receive a ScopedJavaSurface from the GPU process
(or from the browser process, in the case of the single-process).
In the case of the SurfaceTexture being forwarded by the GPU process,
the order in which one should read the functions to follow the
execution flow is as follows:
ScopedSurfaceRequestManager.h --FulfillScopedSurfaceRequest()
ChildProcesServiceImpl.cc -- FulfillScopedSurfaceRequest()
<JNI switch from native to Java>
ChildProcessServiceImpl.Java -- fulfillScopedSurfaceRequest()
IChildProcessCallback.aidl -- fulfillScopedSurfaceRequest()
<Android Binder from the GPU to the browser process>
ChildProcessLauncher.java -- fulfillScopedSurfaceRequest()
<JNI switch from Java to native>
ChilProcessLauncherAndroid.cc -- CompleteScopedSurfaceRequest()
The CL that covers the integration within StreamTexture and
MediaPlayerRenderer is tracked by the same bug, and can be found at
https://codereview.chromium.org/2282633002/.
BUG=627658
TEST=Added UTs, manually verified that it worked in the prototype.
CQ_INCLUDE_TRYBOTS=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
==========
to
==========
Add ScopedSurfaceRequestManager
WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and
the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU
process to a MediaPlayer in the BrowserProcess.
MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a
way to receive surface textures from the GPU.
This change introduces the ScopedSurfaceRequestManager interface, and
concrete implementations to be used from the GPU and Browser processes.
It allows the registration of a callback in the Browser process, which
can then receive a ScopedJavaSurface from the GPU process
(or from the browser process, in the case of the single-process).
In the case of the SurfaceTexture being forwarded by the GPU process,
the order in which one should read the functions to follow the
execution flow is as follows:
ScopedSurfaceRequestManager.h --FulfillScopedSurfaceRequest()
ChildProcesServiceImpl.cc -- FulfillScopedSurfaceRequest()
<JNI switch from native to Java>
ChildProcessServiceImpl.Java -- fulfillScopedSurfaceRequest()
IChildProcessCallback.aidl -- fulfillScopedSurfaceRequest()
<Android Binder from the GPU to the browser process>
ChildProcessLauncher.java -- fulfillScopedSurfaceRequest()
<JNI switch from Java to native>
ChilProcessLauncherAndroid.cc -- CompleteScopedSurfaceRequest()
The CL that covers the integration within StreamTexture and
MediaPlayerRenderer is tracked by the same bug, and can be
found at https://codereview.chromium.org/2282633002/.
BUG=627658
TEST=Added UTs, manually verified that it worked in the prototype.
CQ_INCLUDE_TRYBOTS=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
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org, liberato@chromium.org - dalecurtis@google.com
=> liberato
Since we're intending for request_id to be used for authentication I'd suggest calling it request_token maybe. "id" is often a sequential counter with no security properties whereas token hints at the latter. https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... File content/browser/android/browser_scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... content/browser/android/browser_scoped_surface_request_manager.cc:24: DCHECK(request_callbacks_.find(request_id) == request_callbacks_.end()); Can use .count() for this https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... content/browser/android/browser_scoped_surface_request_manager.cc:29: BrowserScopedSurfaceRequestManager::GetAndUnregisterScopedSurfaceRequest( I don't see the need for both this one and the internal one? https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... File content/browser/android/browser_scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... content/browser/android/browser_scoped_surface_request_manager.h:32: // Tries to callback the request identified by |request_id|. what about "Runs the request callback identified by |request_id| if one exists" https://codereview.chromium.org/2285593002/diff/60001/gpu/ipc/common/android/... File gpu/ipc/common/android/scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/60001/gpu/ipc/common/android/... gpu/ipc/common/android/scoped_surface_request_manager.h:23: class GPU_EXPORT ScopedSurfaceRequestManager { I think it might be more understandable if you avoid a common interface for both gpu and browser sides if they can't be used symmetrically. i.e., you can't use this interface today without knowing which process you're in and which direction the surfaces and requests are flowing. Which means you can probably get rid of interfaces at all. Just have the concrete browser side one. And something for the gpu side to fulfill requests. (maybe just a function/callback since it's only one call).
I changed the ScopedSurfaceRequestManager into ScopedSurfaceRequestConduit and simplified the interface. This allowed me to rename BrowserScopedSurfaceRequestManager into ScopedSurfaceRequestManager. PTAL! :) https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... File content/browser/android/browser_scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... content/browser/android/browser_scoped_surface_request_manager.cc:24: DCHECK(request_callbacks_.find(request_id) == request_callbacks_.end()); On 2016/08/26 22:01:32, watk wrote: > Can use .count() for this Done. https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... content/browser/android/browser_scoped_surface_request_manager.cc:29: BrowserScopedSurfaceRequestManager::GetAndUnregisterScopedSurfaceRequest( On 2016/08/26 22:01:32, watk wrote: > I don't see the need for both this one and the internal one? I added an internal one, so I could lock in the entirety of FulfillScopedSurfaceRequest's scope. You might say that we only want to lock when actually changing |request_callbacks_| and I am fine with that. My line of thinking was that, since any object that registers a request is responsible for un-registering its unfulfilled requests before its destruction, locking at the "FulfillRequest" scope (which is used in the single process case) would be making extra sure that the object we callback the request on is still alive. Any thoughts as to whether we should lock when fulfilling requests in single process mode? https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... File content/browser/android/browser_scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... content/browser/android/browser_scoped_surface_request_manager.h:32: // Tries to callback the request identified by |request_id|. On 2016/08/26 22:01:32, watk wrote: > what about "Runs the request callback identified by |request_id| if one exists" Done. https://codereview.chromium.org/2285593002/diff/60001/gpu/ipc/common/android/... File gpu/ipc/common/android/scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/60001/gpu/ipc/common/android/... gpu/ipc/common/android/scoped_surface_request_manager.h:23: class GPU_EXPORT ScopedSurfaceRequestManager { On 2016/08/26 22:01:32, watk wrote: > I think it might be more understandable if you avoid a common interface for both > gpu and browser sides if they can't be used symmetrically. i.e., you can't use > this interface today without knowing which process you're in and which direction > the surfaces and requests are flowing. > > Which means you can probably get rid of interfaces at all. Just have the > concrete browser side one. And something for the gpu side to fulfill requests. > (maybe just a function/callback since it's only one call). Updated. LMK if this is closer to what you had in mind.
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... File content/browser/android/browser_scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... content/browser/android/browser_scoped_surface_request_manager.cc:29: BrowserScopedSurfaceRequestManager::GetAndUnregisterScopedSurfaceRequest( On 2016/08/29 18:30:46, ThomasGuilbert wrote: > On 2016/08/26 22:01:32, watk wrote: > > I don't see the need for both this one and the internal one? > > I added an internal one, so I could lock in the entirety of > FulfillScopedSurfaceRequest's scope. > > You might say that we only want to lock when actually changing > |request_callbacks_| and I am fine with that. > > My line of thinking was that, since any object that registers a request is > responsible for un-registering its unfulfilled requests before its destruction, > locking at the "FulfillRequest" scope (which is used in the single process case) > would be making extra sure that the object we callback the request on is still > alive. > > Any thoughts as to whether we should lock when fulfilling requests in single > process mode? Ah, gotcha. Yeah, then I think what you have is best. I slightly prefer the _Locked suffix instead of Internal, but up to you. Do we need to expose the "Get" version in the public interface? Probably just UnregisterRequest would make the api clearer. https://codereview.chromium.org/2285593002/diff/60001/gpu/ipc/common/android/... File gpu/ipc/common/android/scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/60001/gpu/ipc/common/android/... gpu/ipc/common/android/scoped_surface_request_manager.h:23: class GPU_EXPORT ScopedSurfaceRequestManager { On 2016/08/29 18:30:46, ThomasGuilbert wrote: > On 2016/08/26 22:01:32, watk wrote: > > I think it might be more understandable if you avoid a common interface for > both > > gpu and browser sides if they can't be used symmetrically. i.e., you can't use > > this interface today without knowing which process you're in and which > direction > > the surfaces and requests are flowing. > > > > Which means you can probably get rid of interfaces at all. Just have the > > concrete browser side one. And something for the gpu side to fulfill requests. > > > (maybe just a function/callback since it's only one call). > > Updated. LMK if this is closer to what you had in mind. I like it! https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/child_process_launcher_android.cc:104: } Two things: 1) why not call FulfillRequest? 2) doesn't this have the race condition you were worried about in fulfill? https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/child_process_launcher_android.cc:264: BrowserThread::UI, FROM_HERE, Do we have guarantees about which thread MPR runs on? If we do, maybe we should remove locking from SSRM and add DCHECKS instead? Seems easier to reason about if we don't need locking in SSRM. i.e., you would make Fulfill post to itself on the UI thread if it were not called on the UI thread. And Register and Unregister would only be allowed to be called on the UI thread. https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/scoped_surface_request_manager.cc:22: request_callbacks_.insert(std::make_pair(request_id, request_cb)); Seems like the request token should be generated here. It would also be a good place to make it clear that the token is used for authentication. https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... File content/browser/android/scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/scoped_surface_request_manager.h:25: void RegisterScopedSurfaceRequest(uint64_t request_id, Without comments it kinda sounds like this gets you a surface without you doing anything. But the caller is responsible for organizing for the surface to be delivered. https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/scoped_surface_request_manager.h:38: // Runs the callback on the current thread. Not only on the current thread but synchronously. (Unless you implemented logic to post to self). https://codereview.chromium.org/2285593002/diff/80001/gpu/ipc/common/android/... File gpu/ipc/common/android/scoped_surface_request_conduit.cc (right): https://codereview.chromium.org/2285593002/diff/80001/gpu/ipc/common/android/... gpu/ipc/common/android/scoped_surface_request_conduit.cc:7: #include "base/logging.h" delete https://codereview.chromium.org/2285593002/diff/80001/gpu/ipc/common/android/... File gpu/ipc/common/android/scoped_surface_request_conduit.h (right): https://codereview.chromium.org/2285593002/diff/80001/gpu/ipc/common/android/... gpu/ipc/common/android/scoped_surface_request_conduit.h:10: #include "base/callback_forward.h" Delete https://codereview.chromium.org/2285593002/diff/80001/gpu/ipc/common/android/... gpu/ipc/common/android/scoped_surface_request_conduit.h:15: class ScopedJavaSurface; Delete https://codereview.chromium.org/2285593002/diff/80001/gpu/ipc/common/android/... gpu/ipc/common/android/scoped_surface_request_conduit.h:20: // Defines an interface that allows the registration of gl::ScopedJavaSurface Out of date. nit: you can drop the "Defines an interface that"
sorry for the delay. watk's comments covered what i was going to say -- mostly, "no need to make this a two-way interface". i'm running late today, but will get back to this tomorrow morning. thanks -fl https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... File content/browser/android/browser_scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... content/browser/android/browser_scoped_surface_request_manager.h:36: gl::SurfaceTexture* surface_texture) override; it's a little unexpected that this is a SurfaceTexture. I expected a ScopedJavaSurface&, based on the name of the class. it's more general purpose, but also less convenient to use. https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/scoped_surface_request_manager.cc:22: request_callbacks_.insert(std::make_pair(request_id, request_cb)); On 2016/08/29 19:42:26, watk wrote: > Seems like the request token should be generated here. It would also be a good > place to make it clear that the token is used for authentication. should also check with chrome security if int64 is sufficient.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sorry for the delay. -fl https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/child_process_launcher_android.cc:95: const base::android::JavaRef<jobject>& surface, perhaps switch |surface| and |request_id|. they're ordered the other way everywhere else. https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/child_process_launcher_android.cc:253: void CompleteScopedSurfaceRequest(JNIEnv* env, /Complete/Fulfill ? https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/child_process_launcher_android.cc:264: BrowserThread::UI, FROM_HERE, On 2016/08/29 19:42:26, watk wrote: > Do we have guarantees about which thread MPR runs on? If we do, maybe we should > remove locking from SSRM and add DCHECKS instead? Seems easier to reason about > if we don't need locking in SSRM. > > i.e., you would make Fulfill post to itself on the UI thread if it were not > called on the UI thread. And Register and Unregister would only be allowed to be > called on the UI thread. conversely, why post here if there will not be any guarantees from the "register" side, once it is added?
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed most comments. If I didn't specifically address it, it might be because the changes are significant in this patch, and the comments are no longer relevant (or need to be re-evaluated). I need to rebase, so there will be at least another patch. PTAL! Thanks, Thomas https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... File content/browser/android/browser_scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... content/browser/android/browser_scoped_surface_request_manager.cc:29: BrowserScopedSurfaceRequestManager::GetAndUnregisterScopedSurfaceRequest( On 2016/08/29 19:42:25, watk wrote: > On 2016/08/29 18:30:46, ThomasGuilbert wrote: > > On 2016/08/26 22:01:32, watk wrote: > > > I don't see the need for both this one and the internal one? > > > > I added an internal one, so I could lock in the entirety of > > FulfillScopedSurfaceRequest's scope. > > > > You might say that we only want to lock when actually changing > > |request_callbacks_| and I am fine with that. > > > > My line of thinking was that, since any object that registers a request is > > responsible for un-registering its unfulfilled requests before its > destruction, > > locking at the "FulfillRequest" scope (which is used in the single process > case) > > would be making extra sure that the object we callback the request on is still > > alive. > > > > Any thoughts as to whether we should lock when fulfilling requests in single > > process mode? > > Ah, gotcha. Yeah, then I think what you have is best. I slightly prefer the > _Locked suffix instead of Internal, but up to you. > > Do we need to expose the "Get" version in the public interface? Probably just > UnregisterRequest would make the api clearer. Ah! Yes, good call on the _Locked suffix. Although, the rest of this discussion might make this obsolete soon. I think from our offline discussion there was some confusion about typing that should be cleared up with the latest patch. https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... File content/browser/android/browser_scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/60001/content/browser/android... content/browser/android/browser_scoped_surface_request_manager.h:36: gl::SurfaceTexture* surface_texture) override; On 2016/08/29 21:10:19, liberato wrote: > it's a little unexpected that this is a SurfaceTexture. I expected a > ScopedJavaSurface&, based on the name of the class. > > it's more general purpose, but also less convenient to use. Updated! https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/child_process_launcher_android.cc:95: const base::android::JavaRef<jobject>& surface, On 2016/08/30 17:58:35, liberato wrote: > perhaps switch |surface| and |request_id|. they're ordered the other way > everywhere else. Deleted this method. https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/child_process_launcher_android.cc:104: } On 2016/08/29 19:42:25, watk wrote: > Two things: > 1) why not call FulfillRequest? > 2) doesn't this have the race condition you were worried about in fulfill? 1) Updated to call Fulfill. 2) I have added DCHECKS to make sure we are on the browser UI thread. I think this solves the race conditions I mentioned. https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/child_process_launcher_android.cc:253: void CompleteScopedSurfaceRequest(JNIEnv* env, On 2016/08/30 17:58:35, liberato wrote: > /Complete/Fulfill ? I don't know if it is easier to follow the sequence of calls if the function names are all identical, or not. In any case, I have updated this method's content, and the naming is now slightly different (and clearer IMO), so leaving complete here, but I am open to re-visiting this in the new patch. https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/child_process_launcher_android.cc:264: BrowserThread::UI, FROM_HERE, On 2016/08/29 19:42:26, watk wrote: > Do we have guarantees about which thread MPR runs on? If we do, maybe we should > remove locking from SSRM and add DCHECKS instead? Seems easier to reason about > if we don't need locking in SSRM. > > i.e., you would make Fulfill post to itself on the UI thread if it were not > called on the UI thread. And Register and Unregister would only be allowed to be > called on the UI thread. I think I have addressed this in this new patch, but let me know if I missed something. https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/80001/content/browser/android... content/browser/android/scoped_surface_request_manager.cc:22: request_callbacks_.insert(std::make_pair(request_id, request_cb)); On 2016/08/29 21:10:19, liberato wrote: > On 2016/08/29 19:42:26, watk wrote: > > Seems like the request token should be generated here. It would also be a good > > place to make it clear that the token is used for authentication. > > should also check with chrome security if int64 is sufficient. Done @ generating the token here. I will follow up with chrome security. I know int64 is used here at least: https://cs.chromium.org/chromium/src/cc/surfaces/surface_id.h?q=nonce&sq=pack... https://codereview.chromium.org/2285593002/diff/80001/gpu/ipc/common/android/... File gpu/ipc/common/android/scoped_surface_request_conduit.cc (right): https://codereview.chromium.org/2285593002/diff/80001/gpu/ipc/common/android/... gpu/ipc/common/android/scoped_surface_request_conduit.cc:7: #include "base/logging.h" On 2016/08/29 19:42:26, watk wrote: > delete Needed for DCHECK. https://codereview.chromium.org/2285593002/diff/80001/gpu/ipc/common/android/... File gpu/ipc/common/android/scoped_surface_request_conduit.h (right): https://codereview.chromium.org/2285593002/diff/80001/gpu/ipc/common/android/... gpu/ipc/common/android/scoped_surface_request_conduit.h:10: #include "base/callback_forward.h" On 2016/08/29 19:42:26, watk wrote: > Delete Done. https://codereview.chromium.org/2285593002/diff/80001/gpu/ipc/common/android/... gpu/ipc/common/android/scoped_surface_request_conduit.h:15: class ScopedJavaSurface; On 2016/08/29 19:42:27, watk wrote: > Delete Done. https://codereview.chromium.org/2285593002/diff/80001/gpu/ipc/common/android/... gpu/ipc/common/android/scoped_surface_request_conduit.h:20: // Defines an interface that allows the registration of gl::ScopedJavaSurface On 2016/08/29 19:42:26, watk wrote: > Out of date. nit: you can drop the "Defines an interface that" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This looks great https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:77: if (!request.is_null()) { remove braces for single line statement https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager.h:28: // It is the request's responsability to check the validity of the final requester's responsibility https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager.h:56: int callback_count_for_testing() { return request_callbacks_.size(); } I think this would be better named request_count_for_testing https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... File content/browser/android/scoped_surface_request_manager_unittest.cc (right): https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:24: manager->clear_callbacks_for_testing(); Can you create a new manager instead? https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:52: ScopedSurfaceRequestManager* manager; We usually use the underscore suffix for members in tests too I think https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:72: EXPECT_EQ(0, manager->callback_count_for_testing()); This duplicates the assertion made in RegisterRequest_ShouldSucceed. We should delete it to keep the tests small and focused on testing one thing. I think all the tests below have similarly redundant EXPECTS. https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:88: EXPECT_EQ(manager, manager_other); This test can be EXPECT_EQ(manager, ScopedSurfaceRequestManager::GetInstance()) RegisterRequest_ShouldSucceed covers the rest. https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:130: // Makes sure that unregistering and invalid |request_token| doesn't affect an https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:184: // Makes sure that the code is resilient to empty callbacks. Easier to DCHECK the request is not null IMO https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:203: last_received_request = 0; initialize in the constructor/SetUp https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:207: EXPECT_EQ(0, manager->callback_count_for_testing()); delete
PTAL. https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:77: if (!request.is_null()) { On 2016/08/31 00:47:35, watk wrote: > remove braces for single line statement Done. https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager.h:28: // It is the request's responsability to check the validity of the final On 2016/08/31 00:47:35, watk wrote: > requester's responsibility Done. https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager.h:56: int callback_count_for_testing() { return request_callbacks_.size(); } On 2016/08/31 00:47:35, watk wrote: > I think this would be better named request_count_for_testing Done. https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... File content/browser/android/scoped_surface_request_manager_unittest.cc (right): https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:24: manager->clear_callbacks_for_testing(); On 2016/08/31 00:47:36, watk wrote: > Can you create a new manager instead? Not really, the constructor is private. I would have to create a for_testing() which would be equivalent. https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:52: ScopedSurfaceRequestManager* manager; On 2016/08/31 00:47:35, watk wrote: > We usually use the underscore suffix for members in tests too I think Interesting. TIL! In my mind underscores were only for private vars, but I double checked and you are right. https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:72: EXPECT_EQ(0, manager->callback_count_for_testing()); On 2016/08/31 00:47:35, watk wrote: > This duplicates the assertion made in RegisterRequest_ShouldSucceed. We should > delete it to keep the tests small and focused on testing one thing. > > I think all the tests below have similarly redundant EXPECTS. Done. https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:88: EXPECT_EQ(manager, manager_other); On 2016/08/31 00:47:36, watk wrote: > This test can be EXPECT_EQ(manager, ScopedSurfaceRequestManager::GetInstance()) > > RegisterRequest_ShouldSucceed covers the rest. Done. https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:130: // Makes sure that unregistering and invalid |request_token| doesn't affect On 2016/08/31 00:47:36, watk wrote: > an Done. https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:184: // Makes sure that the code is resilient to empty callbacks. On 2016/08/31 00:47:35, watk wrote: > Easier to DCHECK the request is not null IMO I added a DCHECK in the Register(), and replaced the "if(!request.is_null())" by a DCHECK in Fulfill(). Let me know if this is not what you had in mind. request should never be null, but I am just a bit worried that, if it is, we will cause a crash in the browser process. Should I keep the "if" instead of the DCHECK in Fulfill()? https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:203: last_received_request = 0; On 2016/08/31 00:47:35, watk wrote: > initialize in the constructor/SetUp Done. https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:207: EXPECT_EQ(0, manager->callback_count_for_testing()); On 2016/08/31 00:47:35, watk wrote: > delete I feel like this one should stay, because, without looking at the .cc file, there is no guarantee that the manager uses Fulfill() (and it could directly access the request map, without unregistering the request).
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... File content/browser/android/scoped_surface_request_manager_unittest.cc (right): https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:184: // Makes sure that the code is resilient to empty callbacks. On 2016/08/31 17:58:56, ThomasGuilbert wrote: > On 2016/08/31 00:47:35, watk wrote: > > Easier to DCHECK the request is not null IMO > > I added a DCHECK in the Register(), and replaced the "if(!request.is_null())" by > a DCHECK in Fulfill(). Let me know if this is not what you had in mind. > > request should never be null, but I am just a bit worried that, if it is, we > will cause a crash in the browser process. > > Should I keep the "if" instead of the DCHECK in Fulfill()? DCHECK in register SG. Yes, I'd say keep the if in Fulfill since if a request is unregistered it will be null. https://codereview.chromium.org/2285593002/diff/120001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/120001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:78: DCHECK(!request_cb.is_null()); Yeah, I think you were right that this one should be an if instead.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from
==========
Add ScopedSurfaceRequestManager
WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and
the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU
process to a MediaPlayer in the BrowserProcess.
MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a
way to receive surface textures from the GPU.
This change introduces the ScopedSurfaceRequestManager interface, and
concrete implementations to be used from the GPU and Browser processes.
It allows the registration of a callback in the Browser process, which
can then receive a ScopedJavaSurface from the GPU process
(or from the browser process, in the case of the single-process).
In the case of the SurfaceTexture being forwarded by the GPU process,
the order in which one should read the functions to follow the
execution flow is as follows:
ScopedSurfaceRequestManager.h --FulfillScopedSurfaceRequest()
ChildProcesServiceImpl.cc -- FulfillScopedSurfaceRequest()
<JNI switch from native to Java>
ChildProcessServiceImpl.Java -- fulfillScopedSurfaceRequest()
IChildProcessCallback.aidl -- fulfillScopedSurfaceRequest()
<Android Binder from the GPU to the browser process>
ChildProcessLauncher.java -- fulfillScopedSurfaceRequest()
<JNI switch from Java to native>
ChilProcessLauncherAndroid.cc -- CompleteScopedSurfaceRequest()
The CL that covers the integration within StreamTexture and
MediaPlayerRenderer is tracked by the same bug, and can be
found at https://codereview.chromium.org/2282633002/.
BUG=627658
TEST=Added UTs, manually verified that it worked in the prototype.
CQ_INCLUDE_TRYBOTS=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
==========
to
==========
Add ScopedSurfaceRequestManager
WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and
the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU
process to a MediaPlayer in the BrowserProcess.
MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a
way to receive surface textures from the GPU. This change introduces the
ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit.
The Manager allows the registration of a callback in the Browser
process, which can receive a ScopedJavaSurface. Upon registration,
the manager creates a token that can be used at a later time to
fulfill the request.
The Conduit forwards surface texture to the Manager, using the
issued token to route the ScopedJavaSurface to the right request.
From the GPU process, surfaces are sent via the ChildProcessServices
and the IChildProcessCallback, the same way that EstablishSurfacePeer
currently does. In single process mode, the Manager directly implements
the Conduit interface.
The CL that covers the integration within StreamTexture and
MediaPlayerRenderer is tracked by the same bug, and can be
found at https://codereview.chromium.org/2282633002/.
BUG=627658
TEST=Added UTs, manually verified that it worked in the prototype.
CQ_INCLUDE_TRYBOTS=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
==========
lgtm
tguilbert@chromium.org changed reviewers: + dcheng@chromium.org, dtrainor@chromium.org, sievers@chromium.org
dtrainor, PTAL at content/public/android/* sievers, PTAL at content/browser/* and content/app/* dcheng, PTAL at gpu/ipc/* @dcheng: can you weigh in on whether uint64 is enough crypto randomness for our purposes from a security POV? Otherwise, could you recommend someone that has that signoff authority"? Here is a high level diagram of what the final integration should like (with the blue segments being the ones relevant to this CL): https://www.lucidchart.com/publicSegments/view/a393c7c8-168d-4f76-adab-259012... If you want to follow the flow of the SurfaceTexture --> request, this is the order that you should use: scoped_surface_request_conduit.h child_process_service_impl.cc ChildProcessServiceImpl.java (IChildProcessCallback.aidl) // GPU --> Browser process ChildProcessLauncher.java child_process_launcher_android.cc surface_request_manager.cc Thank you! Thomas https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... File content/browser/android/scoped_surface_request_manager_unittest.cc (right): https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... content/browser/android/scoped_surface_request_manager_unittest.cc:184: // Makes sure that the code is resilient to empty callbacks. On 2016/08/31 18:12:14, watk wrote: > On 2016/08/31 17:58:56, ThomasGuilbert wrote: > > On 2016/08/31 00:47:35, watk wrote: > > > Easier to DCHECK the request is not null IMO > > > > I added a DCHECK in the Register(), and replaced the "if(!request.is_null())" > by > > a DCHECK in Fulfill(). Let me know if this is not what you had in mind. > > > > request should never be null, but I am just a bit worried that, if it is, we > > will cause a crash in the browser process. > > > > Should I keep the "if" instead of the DCHECK in Fulfill()? > > DCHECK in register SG. Yes, I'd say keep the if in Fulfill since if a request is > unregistered it will be null. Oops! Forgot about unregistered CBs returning null, you are right. https://codereview.chromium.org/2285593002/diff/120001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/120001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:78: DCHECK(!request_cb.is_null()); On 2016/08/31 18:12:14, watk wrote: > Yeah, I think you were right that this one should be an if instead. Done.
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/31 20:44:44, ThomasGuilbert wrote: > dtrainor, PTAL at content/public/android/* > sievers, PTAL at content/browser/* and content/app/* > dcheng, PTAL at gpu/ipc/* > > @dcheng: can you weigh in on whether uint64 is enough crypto randomness for our > purposes from a security POV? Otherwise, could you recommend someone that has > that signoff authority"? Let's email chrome-security and cc piman: I'm not sure what the historical motivations for the current gpu mailbox size were, nor the constraints that went into picking it. > > Here is a high level diagram of what the final integration should like (with the > blue segments being the ones relevant to this CL): > https://www.lucidchart.com/publicSegments/view/a393c7c8-168d-4f76-adab-259012... > > > If you want to follow the flow of the SurfaceTexture --> request, this is the > order that you should use: > scoped_surface_request_conduit.h > child_process_service_impl.cc > ChildProcessServiceImpl.java > (IChildProcessCallback.aidl) // GPU --> Browser process > ChildProcessLauncher.java > child_process_launcher_android.cc > surface_request_manager.cc > > Thank you! > Thomas > > https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... > File content/browser/android/scoped_surface_request_manager_unittest.cc (right): > > https://codereview.chromium.org/2285593002/diff/100001/content/browser/androi... > content/browser/android/scoped_surface_request_manager_unittest.cc:184: // Makes > sure that the code is resilient to empty callbacks. > On 2016/08/31 18:12:14, watk wrote: > > On 2016/08/31 17:58:56, ThomasGuilbert wrote: > > > On 2016/08/31 00:47:35, watk wrote: > > > > Easier to DCHECK the request is not null IMO > > > > > > I added a DCHECK in the Register(), and replaced the > "if(!request.is_null())" > > by > > > a DCHECK in Fulfill(). Let me know if this is not what you had in mind. > > > > > > request should never be null, but I am just a bit worried that, if it is, we > > > will cause a crash in the browser process. > > > > > > Should I keep the "if" instead of the DCHECK in Fulfill()? > > > > DCHECK in register SG. Yes, I'd say keep the if in Fulfill since if a request > is > > unregistered it will be null. > > Oops! Forgot about unregistered CBs returning null, you are right. > > https://codereview.chromium.org/2285593002/diff/120001/content/browser/androi... > File content/browser/android/scoped_surface_request_manager.cc (right): > > https://codereview.chromium.org/2285593002/diff/120001/content/browser/androi... > content/browser/android/scoped_surface_request_manager.cc:78: > DCHECK(!request_cb.is_null()); > On 2016/08/31 18:12:14, watk wrote: > > Yeah, I think you were right that this one should be an if instead. > > Done.
content/public/android lgtm
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by tguilbert@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, dtrainor@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/2285593002/#ps200001 (title: "Remove media token. Switch to base64 string")
The CQ bit was unchecked by tguilbert@chromium.org
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ========== to ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented as a base64 string, in order to improve ease of serialization accross IPC, Mojo, JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ==========
Description was changed from ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented as a base64 string, in order to improve ease of serialization accross IPC, Mojo, JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ========== to ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented as a base64 string, in order to facilitate serialization accross IPC, Mojo, JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ==========
tguilbert@chromium.org changed reviewers: + nick@chromium.org
Security recommended 128 bits as a token size. The discussion also sparked the idea of adding a 128 bit unguessable token as a base class. See crbug.com/643857. Until that base token class is available, I have represented the 128 bit token as a base64 string, because it was the most easily serializeable representation accross IPC+Mojo+JNI+AIDL. @dcheng, can you PTAL at gpu/ipc/*? Thanks!
How much time do you think it'd take to implement the unguessable token? In general, we'd prefer to avoid turning things into base64 strings and then pulling them back out: in addition, we'd have to check for decode failure, etc. Just implementing the base guessable token would eliminate all these issues and make this review much simpler. https://codereview.chromium.org/2285593002/diff/220001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/220001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:38: std::string request_token) { Strings (with few exceptions) should be passed by const ref. https://codereview.chromium.org/2285593002/diff/220001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:47: DCHECK(kPaddedStringTokenSize == request_token.length()); Nit: DCHECK_EQ, but this is not a safe assumption, since this token can come from a less trusted process, no?
nick@chromium.org changed reviewers: - nick@chromium.org
@dcheng I uploaded a tentative CL of what the unguessable token might look like (https://codereview.chromium.org/2333443002/). For the moment the name is simply "base::Nonce", but the name is still subject to change. (btw, I spoke with danakj beforehand to confirm base/ was an acceptable place to add this class) I updated this CL to use base::Nonce, and also updated its child CL. My only concern is from converting base::Nonce to byte[] and back in JNI/AIDL. The aidl file now has a byte[] as an "in parameter". I check that the size the byte[] matches base::Nonce's in the browser process, but would it be better to send two uint64 instead? I will focus on landing base::Nonce on Monday. Is there anything else I should address in the mean time?
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
On 2016/09/10 09:50:48, ThomasGuilbert wrote: > @dcheng I uploaded a tentative CL of what the unguessable token might look like > (https://codereview.chromium.org/2333443002/). For the moment the name is simply > "base::Nonce", but the name is still subject to change. > (btw, I spoke with danakj beforehand to confirm base/ was an acceptable place to > add this class) > > I updated this CL to use base::Nonce, and also updated its child CL. > > My only concern is from converting base::Nonce to byte[] and back in JNI/AIDL. > The aidl file now has a byte[] as an "in parameter". I check that the size the > byte[] matches base::Nonce's in the browser process, but would it be better to > send two uint64 instead? > > I will focus on landing base::Nonce on Monday. Is there anything else I should > address in the mean time? Would it be hard to use 2x uint64_ts for the Java plumbing? Then we won't need to verify that the size of the received byte array is correct, which might make it a bit simpler.
@dcheng I wasn't hard to change it to two longs. That is how I had implemented before changing to the base64 string token.
Description was changed from ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented as a base64 string, in order to facilitate serialization accross IPC, Mojo, JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ========== to ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented by base::Nonce class, which supports serialization accross IPC and Mojo, and is easy to send as two uint64_t through JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ==========
lgtm w/ a few nits https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... content/browser/android/child_process_launcher_android.cc:249: gl::ScopedJavaSurface scoped_surface(jsurface); nit: Doesn't the ScopedJavaSurface constructor take the |surface| directly (it takes a JavaRef<> const-ref)? https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:61: gl::SurfaceTexture* surface_texture) { nit: should be |const gl::SurfaceTexture*| https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:69: if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) { You might want to DCHECK() that this is *not* called on the UI thread. That could cause deadlocks if the GPU process synchronously calls back onto the UI thread. https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... content/browser/android/scoped_surface_request_manager.h:30: // It is the requester's responsability to check the validity of the final nit: s/responsability/responsibility https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... content/browser/android/scoped_surface_request_manager.h:44: // Can be called from any thread. The request will be run synchonously on the see comment in implementation https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... content/browser/android/scoped_surface_request_manager.h:44: // Can be called from any thread. The request will be run synchonously on the nit: s/synchonously/synchronously
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
base::UnguessableToken has landed. PTAL. Thanks! https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... content/browser/android/child_process_launcher_android.cc:249: gl::ScopedJavaSurface scoped_surface(jsurface); On 2016/09/13 23:25:56, sievers (slow) wrote: > nit: Doesn't the ScopedJavaSurface constructor take the |surface| directly (it > takes a JavaRef<> const-ref)? You are right. Leftover from previous iterations. TY! https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:61: gl::SurfaceTexture* surface_texture) { On 2016/09/13 23:25:56, sievers (slow) wrote: > nit: should be |const gl::SurfaceTexture*| Done. https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:69: if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) { On 2016/09/13 23:25:56, sievers (slow) wrote: > You might want to DCHECK() that this is *not* called on the UI thread. > That could cause deadlocks if the GPU process synchronously calls back onto the > UI thread. I changed this to always post onto the UI thread, and then to complete the request. https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... content/browser/android/scoped_surface_request_manager.h:30: // It is the requester's responsability to check the validity of the final On 2016/09/13 23:25:56, sievers (slow) wrote: > nit: s/responsability/responsibility Done. https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi... content/browser/android/scoped_surface_request_manager.h:44: // Can be called from any thread. The request will be run synchonously on the On 2016/09/13 23:25:56, sievers (slow) wrote: > nit: s/synchonously/synchronously Done. https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... content/browser/android/child_process_launcher_android.cc:247: if (request_token_high == 0 && request_token_low == 0) Thoughts on whether this should be logged? https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:74: void ScopedSurfaceRequestManager::CompleteRequestOnUiThread( Style question because I didn't find any official guidance on 2 letter acronyms: CompleteRequestOnUIThread or CompleteRequestOnUiThread?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
gpu/ipc LGTM with a few nits https://codereview.chromium.org/2285593002/diff/280001/content/app/android/ch... File content/app/android/child_process_service_impl.cc (right): https://codereview.chromium.org/2285593002/diff/280001/content/app/android/ch... content/app/android/child_process_service_impl.cc:55: gpu::ScopedSurfaceRequestConduit::SetInstance(NULL); Nit: nullptr https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... content/browser/android/child_process_launcher_android.cc:247: if (request_token_high == 0 && request_token_low == 0) On 2016/09/20 03:08:40, ThomasGuilbert wrote: > Thoughts on whether this should be logged? It's fine to add something like a DLOG(), but it's not necessary. Though in the usual IPC case, bad data might result in a process kill: I'm not sure what the precedent here is (or should be though). https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:71: base::Passed(std::move(surface)))); base::Passed(&surface) is shorthand for this. https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:74: void ScopedSurfaceRequestManager::CompleteRequestOnUiThread( On 2016/09/20 03:08:40, ThomasGuilbert wrote: > Style question because I didn't find any official guidance on 2 letter acronyms: > CompleteRequestOnUIThread or > CompleteRequestOnUiThread? The consensus is that the style guide states this should be Ui. https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... content/browser/android/scoped_surface_request_manager.h:34: ScopedSurfaceRequestCB request_cb); The original guidance was to pass callbacks as const refs. Though the updated docs say to pass by value and use std::move() when ownership is transferred, it's probably best to be consistent with legacy code and use const Callback& for now until we have a clearmigration plan.
Submitting latest patch to CQ. Thank you everyone! https://codereview.chromium.org/2285593002/diff/280001/content/app/android/ch... File content/app/android/child_process_service_impl.cc (right): https://codereview.chromium.org/2285593002/diff/280001/content/app/android/ch... content/app/android/child_process_service_impl.cc:55: gpu::ScopedSurfaceRequestConduit::SetInstance(NULL); On 2016/09/20 06:50:35, dcheng wrote: > Nit: nullptr Done. https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.cc (right): https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:71: base::Passed(std::move(surface)))); On 2016/09/20 06:50:35, dcheng wrote: > base::Passed(&surface) is shorthand for this. TIL TY! https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... content/browser/android/scoped_surface_request_manager.cc:74: void ScopedSurfaceRequestManager::CompleteRequestOnUiThread( On 2016/09/20 06:50:35, dcheng wrote: > On 2016/09/20 03:08:40, ThomasGuilbert wrote: > > Style question because I didn't find any official guidance on 2 letter > acronyms: > > CompleteRequestOnUIThread or > > CompleteRequestOnUiThread? > > The consensus is that the style guide states this should be Ui. Ok, thanks. https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... File content/browser/android/scoped_surface_request_manager.h (right): https://codereview.chromium.org/2285593002/diff/280001/content/browser/androi... content/browser/android/scoped_surface_request_manager.h:34: ScopedSurfaceRequestCB request_cb); On 2016/09/20 06:50:35, dcheng wrote: > The original guidance was to pass callbacks as const refs. Though the updated > docs say to pass by value and use std::move() when ownership is transferred, > it's probably best to be consistent with legacy code and use const Callback& for > now until we have a clearmigration plan. Good to know. Updated.
The CQ bit was checked by tguilbert@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, watk@chromium.org, liberato@chromium.org, sievers@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2285593002/#ps300001 (title: "Addressed latest comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
nick@chromium.org changed reviewers: + nick@chromium.org
rubber stamp lgtm for content/ (sievers lgtmed this while a content owner)
The CQ bit was checked by tguilbert@chromium.org
The CQ bit was unchecked by tguilbert@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented by base::Nonce class, which supports serialization accross IPC and Mojo, and is easy to send as two uint64_t through JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ========== to ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented by base::Nonce class, which supports serialization accross IPC and Mojo, and is easy to send as two uint64_t through JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. Adding palmer@ as TBR, since he is the only owner for aidl files, but is unavailable ATM. TBR=palmer BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ==========
Description was changed from ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented by base::Nonce class, which supports serialization accross IPC and Mojo, and is easy to send as two uint64_t through JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. Adding palmer@ as TBR, since he is the only owner for aidl files, but is unavailable ATM. TBR=palmer BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ========== to ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented by base::Nonce class, which supports serialization accross IPC and Mojo, and is easy to send as two uint64_t through JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. Adding palmer@ as TBR, since he is the only owner for aidl files, but is unavailable ATM. TBR=palmer BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ==========
The CQ bit was checked by tguilbert@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, watk@chromium.org, nick@chromium.org, dcheng@chromium.org, liberato@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2285593002/#ps320001 (title: "Remove unused string headers.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by tguilbert@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, watk@chromium.org, nick@chromium.org, dcheng@chromium.org, liberato@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2285593002/#ps340001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by tguilbert@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, watk@chromium.org, nick@chromium.org, dcheng@chromium.org, liberato@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2285593002/#ps360001 (title: "Added RunUntilIdle() to 2 UTs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented by base::Nonce class, which supports serialization accross IPC and Mojo, and is easy to send as two uint64_t through JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. Adding palmer@ as TBR, since he is the only owner for aidl files, but is unavailable ATM. TBR=palmer BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ========== to ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented by base::Nonce class, which supports serialization accross IPC and Mojo, and is easy to send as two uint64_t through JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. Adding palmer@ as TBR, since he is the only owner for aidl files, but is unavailable ATM. TBR=palmer BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented by base::Nonce class, which supports serialization accross IPC and Mojo, and is easy to send as two uint64_t through JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. Adding palmer@ as TBR, since he is the only owner for aidl files, but is unavailable ATM. TBR=palmer BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 ========== to ========== Add ScopedSurfaceRequestManager WebMediaPlayerAndroid (WMPA) used the EstablishSurfacePeer construct and the BrowserMediaPlayerManager to send a SurfaceTexture from the GPU process to a MediaPlayer in the BrowserProcess. MediaPlayerRender does not use a BrowserMediaPlayerManager, but needs a way to receive surface textures from the GPU. This change introduces the ScopedSurfaceRequestManager and the ScopedSurfaceRequestConduit. The Manager allows the registration of a callback in the Browser process, which can receive a ScopedJavaSurface. Upon registration, the manager creates a token that can be used at a later time to fulfill the request. The Conduit forwards surface texture to the Manager, using the issued token to route the ScopedJavaSurface to the right request. From the GPU process, surfaces are sent via the ChildProcessServices and the IChildProcessCallback, the same way that EstablishSurfacePeer currently does. In single process mode, the Manager directly implements the Conduit interface. Security recommended 128 bits as the token size. It is represented by base::Nonce class, which supports serialization accross IPC and Mojo, and is easy to send as two uint64_t through JNI and AIDL. The CL that covers the integration within StreamTexture and MediaPlayerRenderer is tracked by the same bug, and can be found at https://codereview.chromium.org/2282633002/. Adding palmer@ as TBR, since he is the only owner for aidl files, but is unavailable ATM. TBR=palmer BUG=627658 TEST=Added UTs, manually verified that it worked in the prototype. CQ_INCLUDE_TRYBOTS=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 Committed: https://crrev.com/f08485bfd9514a905dda4ca23eca02e4f1f3d72c Cr-Commit-Position: refs/heads/master@{#419971} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/f08485bfd9514a905dda4ca23eca02e4f1f3d72c Cr-Commit-Position: refs/heads/master@{#419971} |
