Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2282633002:
Integrate Surface requests with MediaPlayerRenderer (Closed)
|
Created:
4 years, 3 months ago by tguilbert Modified:
4 years, 2 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, Aaron Boodman, posciak+watch_chromium.org, avayvod+watch_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, alokp+watch_chromium.org, piman+watch_chromium.org, darin (slow to review), mlamouri+watch-media_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlumb StreamTexture to MediaPlayerRenderer
The ScopedSurfaceRequestManager and ScopedSurfaceRequestConduit
allow classes to register a ScopedJavaSurface request in the browser
process, and to send SurfaceTexture from the GPU process to the
browser process to fulfill those requests.
This change adds the necessary plumbing to send a StreamTexture to
a MediaPlayerRenderer living in the browser, using the
manager/conduit pair.
N.B: In order to prevent the MojoRendererService from taking a
dependency on the MediaPlayerRenderer, the MojoRendererService
initiates surface requests via a callback (given at construction time).
BUG=627658
TEST=successfully exercised the new code path in a working 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/ecb3febf6e08a15ceb4b9fa0af86e0a1591f315e
Cr-Commit-Position: refs/heads/master@{#423393}
Patch Set 1 #Patch Set 2 : Updated to match parent CL changes. #Patch Set 3 : Fixed MojoRenderService constructor #
Total comments: 6
Patch Set 4 : Switch token type to base::Nonce #
Total comments: 8
Patch Set 5 : Addressed comments #Patch Set 6 : Fixed merge error #
Total comments: 10
Patch Set 7 : Addressed comments. #
Total comments: 6
Patch Set 8 : Addressed comments. #
Total comments: 6
Patch Set 9 : Clarified comment #
Total comments: 2
Patch Set 10 : Updated InitiateScopedSurfaceRequest #
Total comments: 8
Patch Set 11 : Added |binding_| to MojoRendererService #Patch Set 12 : Added ReportBadMessage #Patch Set 13 : Fixed punctuation. #
Total comments: 4
Patch Set 14 : Addressed last comments #Dependent Patchsets: Messages
Total messages: 74 (45 generated)
Description was changed from ========== Plumb SurfaceTexture to MediaPlayerRenderer This CL is here only for context for https://codereview.chromium.org/2285593002/ The contents of the CL, and the description of will be updated. BUG=627658 ========== to ========== Plumb SurfaceTexture to MediaPlayerRenderer This CL is here only for context for https://codereview.chromium.org/2285593002/ The contents of the CL, and the description of will be updated. BUG=627658 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Plumb SurfaceTexture to MediaPlayerRenderer This CL is here only for context for https://codereview.chromium.org/2285593002/ The contents of the CL, and the description of will be updated. BUG=627658 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 ========== Plumb SurfaceTexture to MediaPlayerRenderer The ScopedSurfaceRequestManager and ScopedSurfaceRequestConduit allow classes to register a ScopedJavaSurface request in the browser process, and to send SurfaceTexture from the GPU process to the browser process to fulfill those requests. This change adds the necessary plumbing to send a StreamTexture to a MediaPlayerRenderer living in the browser, using the manager/conduit pair. N.B: In order to prevent the MojoRendererService from taking a dependency on the MediaPlayerRenderer, the MojoRendererService initiates surface requests via a callback (given at construction time). BUG=627658 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 ========== Plumb SurfaceTexture to MediaPlayerRenderer The ScopedSurfaceRequestManager and ScopedSurfaceRequestConduit allow classes to register a ScopedJavaSurface request in the browser process, and to send SurfaceTexture from the GPU process to the browser process to fulfill those requests. This change adds the necessary plumbing to send a StreamTexture to a MediaPlayerRenderer living in the browser, using the manager/conduit pair. N.B: In order to prevent the MojoRendererService from taking a dependency on the MediaPlayerRenderer, the MojoRendererService initiates surface requests via a callback (given at construction time). BUG=627658 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 ========== Plumb StreamTexture to MediaPlayerRenderer The ScopedSurfaceRequestManager and ScopedSurfaceRequestConduit allow classes to register a ScopedJavaSurface request in the browser process, and to send SurfaceTexture from the GPU process to the browser process to fulfill those requests. This change adds the necessary plumbing to send a StreamTexture to a MediaPlayerRenderer living in the browser, using the manager/conduit pair. N.B: In order to prevent the MojoRendererService from taking a dependency on the MediaPlayerRenderer, the MojoRendererService initiates surface requests via a callback (given at construction time). BUG=627658 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: + liberato@chromium.org, watk@chromium.org
This CL is dependent on the ScopedSurfaceRequestManager (here https://codereview.chromium.org/2285593002/). I think that that review is mostly stable, and that it is safe to start reviewing this one. PTAL and let me know if there are any obvious things you would like to see changed. Otherwise, you can hold off on doing a deep review until https://codereview.chromium.org/2285593002/ is closed. Thanks! Thomas
Looks fine to me. (It makes me sad that we have to change this many files to plumb a new call through..) https://codereview.chromium.org/2282633002/diff/40001/content/browser/media/a... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2282633002/diff/40001/content/browser/media/a... content/browser/media/android/media_player_renderer.cc:124: return surface_request_id_; token https://codereview.chromium.org/2282633002/diff/40001/content/renderer/media/... File content/renderer/media/android/media_player_renderer_client.cc (right): https://codereview.chromium.org/2282633002/diff/40001/content/renderer/media/... content/renderer/media/android/media_player_renderer_client.cc:65: // No request was registered in the browser. I would instead write why this is NOTREACHED(). You might also consider renaming success to "supported" or something along those lines. Because if the renderer supports the call, it always succeeds. https://codereview.chromium.org/2282633002/diff/40001/media/mojo/clients/mojo... File media/mojo/clients/mojo_renderer_unittest.cc (right): https://codereview.chromium.org/2282633002/diff/40001/media/mojo/clients/mojo... media/mojo/clients/mojo_renderer_unittest.cc:58: base::Callback<uint64_t()>()); InitiateSurfaceRequestCB() for readability?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...) 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_...)
sorry for the delay. -fl https://codereview.chromium.org/2282633002/diff/60001/content/browser/media/a... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2282633002/diff/60001/content/browser/media/a... content/browser/media/android/media_player_renderer.cc:111: media_player_->SetVideoSurface(std::move(surface)); the ScopedSurfaceRequestManager automatically unregisters the request, right? as in, a (broken / compromised) renderer might actually disobey the contract and re-use the nonce. https://codereview.chromium.org/2282633002/diff/60001/content/renderer/media/... File content/renderer/media/android/media_player_renderer_client.cc (right): https://codereview.chromium.org/2282633002/diff/60001/content/renderer/media/... content/renderer/media/android/media_player_renderer_client.cc:64: if (!supported) { can |supported| be replaced by a null nonce? i assume that the whole point of |supported| is so that other mojo renderers don't need to worry about implementing it. or, alternatively, as part of the mojo renderer setup, could the remote side indicate to the local side that it doesn't support it, so that the local side can fail / return false immediately (without round trip) when calling InitiateScopedSurfaceRequest? https://codereview.chromium.org/2282633002/diff/60001/content/renderer/media/... content/renderer/media/android/media_player_renderer_client.cc:68: // sprocess likely isn't a MediaPlayerRenderer. sprocess? https://codereview.chromium.org/2282633002/diff/60001/media/mojo/interfaces/r... File media/mojo/interfaces/renderer.mojom (right): https://codereview.chromium.org/2282633002/diff/60001/media/mojo/interfaces/r... media/mojo/interfaces/renderer.mojom:40: // its token. please document |supported|
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 tguilbert@chromium.org
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Updated with base::UnguessableToken. The parent CL is in the CQ ATM as well. https://codereview.chromium.org/2282633002/diff/40001/content/browser/media/a... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2282633002/diff/40001/content/browser/media/a... content/browser/media/android/media_player_renderer.cc:124: return surface_request_id_; On 2016/09/01 21:58:58, watk wrote: > token Done. https://codereview.chromium.org/2282633002/diff/40001/content/renderer/media/... File content/renderer/media/android/media_player_renderer_client.cc (right): https://codereview.chromium.org/2282633002/diff/40001/content/renderer/media/... content/renderer/media/android/media_player_renderer_client.cc:65: // No request was registered in the browser. On 2016/09/01 21:58:58, watk wrote: > I would instead write why this is NOTREACHED(). > > You might also consider renaming success to "supported" or something along those > lines. Because if the renderer supports the call, it always succeeds. Done. https://codereview.chromium.org/2282633002/diff/40001/media/mojo/clients/mojo... File media/mojo/clients/mojo_renderer_unittest.cc (right): https://codereview.chromium.org/2282633002/diff/40001/media/mojo/clients/mojo... media/mojo/clients/mojo_renderer_unittest.cc:58: base::Callback<uint64_t()>()); On 2016/09/01 21:58:58, watk wrote: > InitiateSurfaceRequestCB() for readability? Done. https://codereview.chromium.org/2282633002/diff/60001/content/browser/media/a... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2282633002/diff/60001/content/browser/media/a... content/browser/media/android/media_player_renderer.cc:111: media_player_->SetVideoSurface(std::move(surface)); On 2016/09/15 19:40:40, liberato wrote: > the ScopedSurfaceRequestManager automatically unregisters the request, right? > > as in, a (broken / compromised) renderer might actually disobey the contract and > re-use the nonce. Yes. Request tokens are one time uses, and the browser automatically unregisters requests if an attempt to complete them was made. https://codereview.chromium.org/2282633002/diff/60001/content/renderer/media/... File content/renderer/media/android/media_player_renderer_client.cc (right): https://codereview.chromium.org/2282633002/diff/60001/content/renderer/media/... content/renderer/media/android/media_player_renderer_client.cc:64: if (!supported) { On 2016/09/15 19:40:40, liberato wrote: > can |supported| be replaced by a null nonce? > > i assume that the whole point of |supported| is so that other mojo renderers > don't need to worry about implementing it. > > or, alternatively, as part of the mojo renderer setup, could the remote side > indicate to the local side that it doesn't support it, so that the local side > can fail / return false immediately (without round trip) when calling > InitiateScopedSurfaceRequest? Because it's illegal to send an empty UnguessableToken across processes, I am now using base::Optional's nullopt to indicate the request was unsupported. https://codereview.chromium.org/2282633002/diff/60001/content/renderer/media/... content/renderer/media/android/media_player_renderer_client.cc:68: // sprocess likely isn't a MediaPlayerRenderer. On 2016/09/15 19:40:40, liberato wrote: > sprocess? Its a sbetter svariant of a process :) (fixed.) https://codereview.chromium.org/2282633002/diff/60001/media/mojo/interfaces/r... File media/mojo/interfaces/renderer.mojom (right): https://codereview.chromium.org/2282633002/diff/60001/media/mojo/interfaces/r... media/mojo/interfaces/renderer.mojom:40: // its token. On 2016/09/15 19:40:40, liberato wrote: > please document |supported| Removed (but also updated the comment)
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_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #6 (id:100001) has been deleted
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.
lgtm % nits. https://codereview.chromium.org/2282633002/diff/120001/content/browser/media/... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2282633002/diff/120001/content/browser/media/... content/browser/media/android/media_player_renderer.cc:111: void MediaPlayerRenderer::CancelScopedSurfaceRequest() { nit: might want to move this to be in the order in the .h file, or at least after Initiate...(). kinda unexpected to have 'cancel' first. https://codereview.chromium.org/2282633002/diff/120001/content/browser/media/... content/browser/media/android/media_player_renderer.cc:112: if (is_waiting_for_surface_) { seems like chromium tends to early-out and save the {} indentation. i.e., "if (!is...) return" https://codereview.chromium.org/2282633002/diff/120001/content/browser/media/... File content/browser/media/android/media_player_renderer.h (right): https://codereview.chromium.org/2282633002/diff/120001/content/browser/media/... content/browser/media/android/media_player_renderer.h:89: base::UnguessableToken InitiateScopedSurfaceRequest(); this could use some comments, including that it cancels any pending request. https://codereview.chromium.org/2282633002/diff/120001/content/browser/media/... content/browser/media/android/media_player_renderer.h:96: void CancelScopedSurfaceRequest(); perhaps comment that it's okay if there isn't any outstanding request. https://codereview.chromium.org/2282633002/diff/120001/media/mojo/clients/moj... File media/mojo/clients/mojo_renderer.h (right): https://codereview.chromium.org/2282633002/diff/120001/media/mojo/clients/moj... media/mojo/clients/mojo_renderer.h:156: // Lock used to serialize access for |time_interpolator_|. time_interpolator_ => media_time_interpolator_. also, perhaps move |media_clock_| out of the way if it's not protected by |lock_|. or, if it is, then it should probably be in the comment.
tguilbert@chromium.org changed reviewers: + nick@chromium.org
+nick can you OWNERS review content/* +dcheng can you security review gpu/ipc/* and media/mojo/interfaces/* (this is the last review in the series of UnguessableToken --> ScopedSurfaceRequest --> rest of the plumbing) PTAL :) Thanks, Thomas https://codereview.chromium.org/2282633002/diff/120001/content/browser/media/... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2282633002/diff/120001/content/browser/media/... content/browser/media/android/media_player_renderer.cc:111: void MediaPlayerRenderer::CancelScopedSurfaceRequest() { On 2016/09/21 14:33:28, liberato wrote: > nit: might want to move this to be in the order in the .h file, or at least > after Initiate...(). kinda unexpected to have 'cancel' first. Done. https://codereview.chromium.org/2282633002/diff/120001/content/browser/media/... content/browser/media/android/media_player_renderer.cc:112: if (is_waiting_for_surface_) { On 2016/09/21 14:33:28, liberato wrote: > seems like chromium tends to early-out and save the {} indentation. i.e., "if > (!is...) return" Done. https://codereview.chromium.org/2282633002/diff/120001/content/browser/media/... File content/browser/media/android/media_player_renderer.h (right): https://codereview.chromium.org/2282633002/diff/120001/content/browser/media/... content/browser/media/android/media_player_renderer.h:89: base::UnguessableToken InitiateScopedSurfaceRequest(); On 2016/09/21 14:33:28, liberato wrote: > this could use some comments, including that it cancels any pending request. Yes, MB! https://codereview.chromium.org/2282633002/diff/120001/content/browser/media/... content/browser/media/android/media_player_renderer.h:96: void CancelScopedSurfaceRequest(); On 2016/09/21 14:33:28, liberato wrote: > perhaps comment that it's okay if there isn't any outstanding request. Agreed. https://codereview.chromium.org/2282633002/diff/120001/media/mojo/clients/moj... File media/mojo/clients/mojo_renderer.h (right): https://codereview.chromium.org/2282633002/diff/120001/media/mojo/clients/moj... media/mojo/clients/mojo_renderer.h:156: // Lock used to serialize access for |time_interpolator_|. On 2016/09/21 14:33:28, liberato wrote: > time_interpolator_ => media_time_interpolator_. also, perhaps move > |media_clock_| out of the way if it's not protected by |lock_|. or, if it is, > then it should probably be in the comment. I think you may have been looking at the patch-patch diff, rather than the base-patch diff. I didn't touch this part of the code, and it only showed up because there was a rebase and a long time between patches. Actual changes in this file: https://codereview.chromium.org/2282633002/diff/120001/media/mojo/clients/moj... Is there anything that you think should actually be adressed in this portion of the code though? I will be happy to tweak it in another review.
tguilbert@chromium.org changed reviewers: + dcheng@chromium.org
Forgot to actually include dcheng. Copy pasting relevant message here: +dcheng can you security review gpu/ipc/* and media/mojo/interfaces/* (this is the last review in the series of UnguessableToken --> ScopedSurfaceRequestManager --> rest of the plumbing)
Description was changed from ========== Plumb StreamTexture to MediaPlayerRenderer The ScopedSurfaceRequestManager and ScopedSurfaceRequestConduit allow classes to register a ScopedJavaSurface request in the browser process, and to send SurfaceTexture from the GPU process to the browser process to fulfill those requests. This change adds the necessary plumbing to send a StreamTexture to a MediaPlayerRenderer living in the browser, using the manager/conduit pair. N.B: In order to prevent the MojoRendererService from taking a dependency on the MediaPlayerRenderer, the MojoRendererService initiates surface requests via a callback (given at construction time). BUG=627658 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 ========== Plumb StreamTexture to MediaPlayerRenderer The ScopedSurfaceRequestManager and ScopedSurfaceRequestConduit allow classes to register a ScopedJavaSurface request in the browser process, and to send SurfaceTexture from the GPU process to the browser process to fulfill those requests. This change adds the necessary plumbing to send a StreamTexture to a MediaPlayerRenderer living in the browser, using the manager/conduit pair. N.B: In order to prevent the MojoRendererService from taking a dependency on the MediaPlayerRenderer, the MojoRendererService initiates surface requests via a callback (given at construction time). BUG=627658 TEST=successfully exercised the new code path in a working 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
-nick because I just realized files in content/browser/media/* and content/renderer/media/* are also owned by media/OWNERS (and that liberato's LGTM is sufficient)
nick@chromium.org changed reviewers: + nick@chromium.org
I'm pretty sure that watk's approval is all you need here (content/browser/media and content/renderer/media are approvable by anyone in https://cs.chromium.org/chromium/src/media/OWNERS)
tguilbert@chromium.org changed reviewers: + kenrb@chromium.org - nick@chromium.org
Gentle ping :)
https://codereview.chromium.org/2282633002/diff/140001/content/renderer/media... File content/renderer/media/android/media_player_renderer_client.cc (right): https://codereview.chromium.org/2282633002/diff/140001/content/renderer/media... content/renderer/media/android/media_player_renderer_client.cc:63: if (request_token == base::nullopt) { if (!request_token) https://codereview.chromium.org/2282633002/diff/140001/media/mojo/clients/moj... File media/mojo/clients/mojo_renderer.h (right): https://codereview.chromium.org/2282633002/diff/140001/media/mojo/clients/moj... media/mojo/clients/mojo_renderer.h:68: ReceiveSurfaceRequestTokenCB receive_request_token_cb); Callbacks are typically passed by const ref rather than by value (even though copying it is relatively lightweight, it's not free). (Strictly speaking, the preferred method going forward will be to pass by value and use std::move() where transferring ownership, but that's not consistent with how it's used in the codebase today. At some point, we're going to have a migration plan and convert a lot of code to this new model though. Something to think about...) https://codereview.chromium.org/2282633002/diff/140001/media/mojo/services/mo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2282633002/diff/140001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:244: if (initiate_surface_request_cb_.is_null()) { Isn't this always going to be true currently? I'm trying to understand how the new code path is reached.
tguilbert@chromium.org changed reviewers: - kenrb@chromium.org
PTAL. Thanks! https://codereview.chromium.org/2282633002/diff/140001/content/renderer/media... File content/renderer/media/android/media_player_renderer_client.cc (right): https://codereview.chromium.org/2282633002/diff/140001/content/renderer/media... content/renderer/media/android/media_player_renderer_client.cc:63: if (request_token == base::nullopt) { On 2016/09/26 17:41:51, dcheng wrote: > if (!request_token) Done. https://codereview.chromium.org/2282633002/diff/140001/media/mojo/clients/moj... File media/mojo/clients/mojo_renderer.h (right): https://codereview.chromium.org/2282633002/diff/140001/media/mojo/clients/moj... media/mojo/clients/mojo_renderer.h:68: ReceiveSurfaceRequestTokenCB receive_request_token_cb); On 2016/09/26 17:41:51, dcheng wrote: > Callbacks are typically passed by const ref rather than by value (even though > copying it is relatively lightweight, it's not free). > > (Strictly speaking, the preferred method going forward will be to pass by value > and use std::move() where transferring ownership, but that's not consistent with > how it's used in the codebase today. At some point, we're going to have a > migration plan and convert a lot of code to this new model though. Something to > think about...) Ah yes! Done. https://codereview.chromium.org/2282633002/diff/140001/media/mojo/services/mo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2282633002/diff/140001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:244: if (initiate_surface_request_cb_.is_null()) { On 2016/09/26 17:41:51, dcheng wrote: > Isn't this always going to be true currently? I'm trying to understand how the > new code path is reached. I will be enabling this new code path in the next CL that I send (behind a compiler flag at first, and then as a replacement for WebMediaPlayerAndroid entirely). The CL in question will register a MojoRenderService at the RenderFrameHost level on Android phones. The MojoRendererService will be given a callback (binding a MediaPlayerRenderer and InitiateScopedSurfaceRequest()) at construction time. The decision whether to construct a normal instance of WebMediaPlayerImpl or one that uses the MediaPlayerRenderer will be made here https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?sq....
On 2016/09/26 19:46:35, ThomasGuilbert wrote: > PTAL. Thanks! > > https://codereview.chromium.org/2282633002/diff/140001/content/renderer/media... > File content/renderer/media/android/media_player_renderer_client.cc (right): > > https://codereview.chromium.org/2282633002/diff/140001/content/renderer/media... > content/renderer/media/android/media_player_renderer_client.cc:63: if > (request_token == base::nullopt) { > On 2016/09/26 17:41:51, dcheng wrote: > > if (!request_token) > > Done. > > https://codereview.chromium.org/2282633002/diff/140001/media/mojo/clients/moj... > File media/mojo/clients/mojo_renderer.h (right): > > https://codereview.chromium.org/2282633002/diff/140001/media/mojo/clients/moj... > media/mojo/clients/mojo_renderer.h:68: ReceiveSurfaceRequestTokenCB > receive_request_token_cb); > On 2016/09/26 17:41:51, dcheng wrote: > > Callbacks are typically passed by const ref rather than by value (even though > > copying it is relatively lightweight, it's not free). > > > > (Strictly speaking, the preferred method going forward will be to pass by > value > > and use std::move() where transferring ownership, but that's not consistent > with > > how it's used in the codebase today. At some point, we're going to have a > > migration plan and convert a lot of code to this new model though. Something > to > > think about...) > > Ah yes! Done. > > https://codereview.chromium.org/2282633002/diff/140001/media/mojo/services/mo... > File media/mojo/services/mojo_renderer_service.cc (right): > > https://codereview.chromium.org/2282633002/diff/140001/media/mojo/services/mo... > media/mojo/services/mojo_renderer_service.cc:244: if > (initiate_surface_request_cb_.is_null()) { > On 2016/09/26 17:41:51, dcheng wrote: > > Isn't this always going to be true currently? I'm trying to understand how the > > new code path is reached. > > I will be enabling this new code path in the next CL that I send (behind a > compiler flag at first, and then as a replacement for WebMediaPlayerAndroid > entirely). > > The CL in question will register a MojoRenderService at the RenderFrameHost > level on Android phones. The MojoRendererService will be given a callback > (binding a MediaPlayerRenderer and InitiateScopedSurfaceRequest()) at > construction time. The decision whether to construct a normal instance of > WebMediaPlayerImpl or one that uses the MediaPlayerRenderer will be made here > https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?sq.... Ping. Thanks!
tguilbert@chromium.org changed reviewers: + ochang@chromium.org, vmiura@chromium.org
+ vmiura, can you OWNERS review gpu/ipc/service/stream_texture_android* + ochang, can you security review gpu/ipc/common/* and mojo/media/interfaces/* Thanks you! Thomas
For mojo changes, just a minor nit and confusion around the semantics that I don't fully understand. https://codereview.chromium.org/2282633002/diff/160001/content/browser/media/... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2282633002/diff/160001/content/browser/media/... content/browser/media/android/media_player_renderer.cc:251: if (!is_waiting_for_surface_) This bit of state looks like it's always going to be in sync with |surface_request_token_.is_empty()|. Would it be possible to just use that directly for determining if there's any work to be done here? https://codereview.chromium.org/2282633002/diff/160001/content/renderer/media... File content/renderer/media/android/media_player_renderer_client.cc (right): https://codereview.chromium.org/2282633002/diff/160001/content/renderer/media... content/renderer/media/android/media_player_renderer_client.cc:63: if (!request_token) { After thinking about this over the weekend... I'm still have trouble wrapping my head around this reply callback. The mojom declaration says this can return nullopt on error. At the same time, this has a NOTREACHED(). 1) Is there a reason why we'd want to call this on the renderer if the MojoRendererService wasn't constructed with a non-null callback? The NOTREACHED() seems to indicate this should never happen. 2) If it only for error cases, does a NOTREACHED() still make sense? What are the errors that could cause this?
I slightly changed the wording in the comment in the Mojom file, but I don't know if it's clear enough. Adding the "Should only be called on Android" also implicitly gives an example of when the InitiateScopedSurfaceRequest is not supported. https://codereview.chromium.org/2282633002/diff/160001/content/browser/media/... File content/browser/media/android/media_player_renderer.cc (right): https://codereview.chromium.org/2282633002/diff/160001/content/browser/media/... content/browser/media/android/media_player_renderer.cc:251: if (!is_waiting_for_surface_) On 2016/10/03 21:11:27, dcheng wrote: > This bit of state looks like it's always going to be in sync with > |surface_request_token_.is_empty()|. Would it be possible to just use that > directly for determining if there's any work to be done here? You are right. Done! https://codereview.chromium.org/2282633002/diff/160001/content/renderer/media... File content/renderer/media/android/media_player_renderer_client.cc (right): https://codereview.chromium.org/2282633002/diff/160001/content/renderer/media... content/renderer/media/android/media_player_renderer_client.cc:63: if (!request_token) { On 2016/10/03 21:11:27, dcheng wrote: > After thinking about this over the weekend... I'm still have trouble wrapping my > head around this reply callback. > > The mojom declaration says this can return nullopt on error. At the same time, > this has a NOTREACHED(). > > 1) Is there a reason why we'd want to call this on the renderer if the > MojoRendererService wasn't constructed with a non-null callback? The > NOTREACHED() seems to indicate this should never happen. > > 2) If it only for error cases, does a NOTREACHED() still make sense? What are > the errors that could cause this? 1) There is no situation where we would want to call OnScopedSurfaceRequest if the media::Renderer* that was given to the MojoRendererService (MRS) at creation time is not a content::MediaPlayerRenderer*. There is no way for the MediaPlayerRendererClient to poll the MRS and to make sure it properly constructed. Currently, the MRS registered at the frame host level will be the only MRS to be constructed with a MediaPlayerRenderer. If we receive an empty nullopt token in OnScopedSurfaceRequested, this is an initialization/configuration error from the developer, rather than an unforseen runtime error. It would therefore be more appropriate to change the comment in the mojom to "configuration error". NOTE: I still haven't sent the CL that actually registers the MRS. 2) There should be no "runtime error" that could cause this. Would it make make sense to clarify the comments in the .mojom and to leave the NOTREACHED() here there?
https://codereview.chromium.org/2282633002/diff/160001/content/renderer/media... File content/renderer/media/android/media_player_renderer_client.cc (right): https://codereview.chromium.org/2282633002/diff/160001/content/renderer/media... content/renderer/media/android/media_player_renderer_client.cc:63: if (!request_token) { On 2016/10/03 22:26:19, ThomasGuilbert wrote: > On 2016/10/03 21:11:27, dcheng wrote: > > After thinking about this over the weekend... I'm still have trouble wrapping > my > > head around this reply callback. > > > > The mojom declaration says this can return nullopt on error. At the same time, > > this has a NOTREACHED(). > > > > 1) Is there a reason why we'd want to call this on the renderer if the > > MojoRendererService wasn't constructed with a non-null callback? The > > NOTREACHED() seems to indicate this should never happen. > > > > 2) If it only for error cases, does a NOTREACHED() still make sense? What are > > the errors that could cause this? > > 1) There is no situation where we would want to call OnScopedSurfaceRequest if > the media::Renderer* that was given to the MojoRendererService (MRS) at creation > time is not a content::MediaPlayerRenderer*. There is no way for the > MediaPlayerRendererClient to poll the MRS and to make sure it properly > constructed. > Currently, the MRS registered at the frame host level will be the only MRS to be > constructed with a MediaPlayerRenderer. > If we receive an empty nullopt token in OnScopedSurfaceRequested, this is an > initialization/configuration error from the developer, rather than an unforseen > runtime error. It would therefore be more appropriate to change the > comment in the mojom to "configuration error". > > NOTE: I still haven't sent the CL that actually registers the MRS. > > 2) There should be no "runtime error" that could cause this. > > Would it make make sense to clarify the comments in the .mojom and to leave the > NOTREACHED() here there? OK, thanks for the explanation. Let's update the mojom to mention this. Given this, I personally prefer to DCHECK that request_token is non-null. To me, when the if () branch is written out explicitly, it indicates that something can happen, whereas DCHECK() is an explicit indication of precondition that should never be violated. https://codereview.chromium.org/2282633002/diff/180001/media/mojo/services/mo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2282633002/diff/180001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:247: // |renderer_| is likely not of type MediaPlayerRenderer. Given the previous comments, does hitting this mean the renderer is misbheaving and should be killed?
https://codereview.chromium.org/2282633002/diff/160001/content/renderer/media... File content/renderer/media/android/media_player_renderer_client.cc (right): https://codereview.chromium.org/2282633002/diff/160001/content/renderer/media... content/renderer/media/android/media_player_renderer_client.cc:63: if (!request_token) { On 2016/10/04 06:28:39, dcheng wrote: > On 2016/10/03 22:26:19, ThomasGuilbert wrote: > > On 2016/10/03 21:11:27, dcheng wrote: > > > After thinking about this over the weekend... I'm still have trouble > wrapping > > my > > > head around this reply callback. > > > > > > The mojom declaration says this can return nullopt on error. At the same > time, > > > this has a NOTREACHED(). > > > > > > 1) Is there a reason why we'd want to call this on the renderer if the > > > MojoRendererService wasn't constructed with a non-null callback? The > > > NOTREACHED() seems to indicate this should never happen. > > > > > > 2) If it only for error cases, does a NOTREACHED() still make sense? What > are > > > the errors that could cause this? > > > > 1) There is no situation where we would want to call OnScopedSurfaceRequest if > > the media::Renderer* that was given to the MojoRendererService (MRS) at > creation > > time is not a content::MediaPlayerRenderer*. There is no way for the > > MediaPlayerRendererClient to poll the MRS and to make sure it properly > > constructed. > > Currently, the MRS registered at the frame host level will be the only MRS to > be > > constructed with a MediaPlayerRenderer. > > If we receive an empty nullopt token in OnScopedSurfaceRequested, this is an > > initialization/configuration error from the developer, rather than an > unforseen > > runtime error. It would therefore be more appropriate to change the > > comment in the mojom to "configuration error". > > > > NOTE: I still haven't sent the CL that actually registers the MRS. > > > > 2) There should be no "runtime error" that could cause this. > > > > Would it make make sense to clarify the comments in the .mojom and to leave > the > > NOTREACHED() here there? > > OK, thanks for the explanation. Let's update the mojom to mention this. > > Given this, I personally prefer to DCHECK that request_token is non-null. To me, > when the if () branch is written out explicitly, it indicates that something can > happen, whereas DCHECK() is an explicit indication of precondition that should > never be violated. Done. (Although, the request_token is no longer a base::Optional) https://codereview.chromium.org/2282633002/diff/180001/media/mojo/services/mo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2282633002/diff/180001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:247: // |renderer_| is likely not of type MediaPlayerRenderer. On 2016/10/04 06:28:39, dcheng wrote: > Given the previous comments, does hitting this mean the renderer is misbheaving > and should be killed? The MojoRendererService can be used in the browser, the GPU and the utility process. I changed this to close the connection (by calling "delete this") because mojo::ReportBadMessage does not work in all of those cases. Is there a better alternative? I also updated the function to no longer use a base::Optional<>
https://codereview.chromium.org/2282633002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2282633002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:248: // This is an unexpected call, and the connection should be closed. Still might be nice to call mojo::ReportBadMessage here (since it will do a process kill if we're in the browser. Outside the browser process, maybe we'll histogram things?) https://codereview.chromium.org/2282633002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:249: delete this; delete this is problematic, since this is wrapped in StrongBinding in the unit test (and presumably will be once it's actually used). I'm actually not sure what a good way to close the associated message pipe would be.
@dcheng FYI, I will be sending out the CL that registers the MojoRendererService on Android phones by EOD (behind a compiler flag at first). It will provide additional context for this review. https://codereview.chromium.org/2282633002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2282633002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:248: // This is an unexpected call, and the connection should be closed. On 2016/10/05 07:18:40, dcheng wrote: > Still might be nice to call mojo::ReportBadMessage here (since it will do a > process kill if we're in the browser. Outside the browser process, maybe we'll > histogram things?) I'm not sure what the proper mojo::ReportBadMessage behavior is in a 'renderer <=> GPU' or a 'renderer <=> utility' connection. That being said, is there a security concern with only closing the connection and not killing the process? I expect that the only time we will actually call this function improperly is at development time, and, otherwise, I don't know if this method increases our attack surface meaningfully. https://codereview.chromium.org/2282633002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:249: delete this; On 2016/10/05 07:18:40, dcheng wrote: > delete this is problematic, since this is wrapped in StrongBinding in the unit > test (and presumably will be once it's actually used). I'm actually not sure > what a good way to close the associated message pipe would be. I thought "delete this" was pretty much the only way to close a pipe from an object that is in a strong binding. There is precedent here at least: https://cs.chromium.org/chromium/src/device/battery/battery_monitor_impl.cc?s...
https://codereview.chromium.org/2282633002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2282633002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:248: // This is an unexpected call, and the connection should be closed. On 2016/10/05 19:20:53, ThomasGuilbert wrote: > On 2016/10/05 07:18:40, dcheng wrote: > > Still might be nice to call mojo::ReportBadMessage here (since it will do a > > process kill if we're in the browser. Outside the browser process, maybe we'll > > histogram things?) > > I'm not sure what the proper mojo::ReportBadMessage behavior is in a 'renderer > <=> GPU' or a 'renderer <=> utility' connection. > > That being said, is there a security concern with only closing the connection > and not killing the process? I expect that the only time we will actually call > this function improperly is at development time, and, otherwise, I don't know if > this method increases our attack surface meaningfully. Right: if it's hooked up, it'll do something useful. And if it's not, it doesn't hurt to call it, at least that's my understanding: it's easy enough to remove calls later, whereas its hard to find and add calls that don't exist yet) https://codereview.chromium.org/2282633002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:249: delete this; On 2016/10/05 19:20:53, ThomasGuilbert wrote: > On 2016/10/05 07:18:40, dcheng wrote: > > delete this is problematic, since this is wrapped in StrongBinding in the unit > > test (and presumably will be once it's actually used). I'm actually not sure > > what a good way to close the associated message pipe would be. > > I thought "delete this" was pretty much the only way to close a pipe from an > object that is in a strong binding. There is precedent here at least: > https://cs.chromium.org/chromium/src/device/battery/battery_monitor_impl.cc?s... I confirmed with rockot@ that this code is unsafe: the StrongBinding has unique ownership of |this|. rockot@ mentioned that MakeStrongBinding returns a WeakPtr to the strong binding, and we can use that instead to close the pipe (which deletes this).
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 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...
PTAL. Thanks! https://codereview.chromium.org/2282633002/diff/200001/media/mojo/services/mo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2282633002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:248: // This is an unexpected call, and the connection should be closed. On 2016/10/05 20:04:13, dcheng wrote: > On 2016/10/05 19:20:53, ThomasGuilbert wrote: > > On 2016/10/05 07:18:40, dcheng wrote: > > > Still might be nice to call mojo::ReportBadMessage here (since it will do a > > > process kill if we're in the browser. Outside the browser process, maybe > we'll > > > histogram things?) > > > > I'm not sure what the proper mojo::ReportBadMessage behavior is in a 'renderer > > <=> GPU' or a 'renderer <=> utility' connection. > > > > That being said, is there a security concern with only closing the connection > > and not killing the process? I expect that the only time we will actually call > > this function improperly is at development time, and, otherwise, I don't know > if > > this method increases our attack surface meaningfully. > > Right: if it's hooked up, it'll do something useful. And if it's not, it doesn't > hurt to call it, at least that's my understanding: it's easy enough to remove > calls later, whereas its hard to find and add calls that don't exist yet) Ok, done! https://codereview.chromium.org/2282633002/diff/200001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:249: delete this; On 2016/10/05 20:04:13, dcheng wrote: > On 2016/10/05 19:20:53, ThomasGuilbert wrote: > > On 2016/10/05 07:18:40, dcheng wrote: > > > delete this is problematic, since this is wrapped in StrongBinding in the > unit > > > test (and presumably will be once it's actually used). I'm actually not sure > > > what a good way to close the associated message pipe would be. > > > > I thought "delete this" was pretty much the only way to close a pipe from an > > object that is in a strong binding. There is precedent here at least: > > > https://cs.chromium.org/chromium/src/device/battery/battery_monitor_impl.cc?s... > > I confirmed with rockot@ that this code is unsafe: the StrongBinding has unique > ownership of |this|. rockot@ mentioned that MakeStrongBinding returns a WeakPtr > to the strong binding, and we can use that instead to close the pipe (which > deletes this). Done.
LGTM https://codereview.chromium.org/2282633002/diff/260001/media/mojo/services/mo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2282633002/diff/260001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:32: MojoRendererService* mojo_renderer_service = new MojoRendererService( Nit: auto service = base::MakeUnique<MojoRendererService>(...); and then just std::move() below. https://codereview.chromium.org/2282633002/diff/260001/media/mojo/services/mo... File media/mojo/services/mojo_renderer_service.h (right): https://codereview.chromium.org/2282633002/diff/260001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.h:45: // NOTE: The MojoRendererService will be uniquely owned by the returned StrongBindingPtr is actually a WeakPtr. The created impl is actually owned by the StrongBinding itself directly, while the StrongBindingImpl provides a safe way to reference back to it.
On 2016/10/05 22:37:13, dcheng wrote: > LGTM > > https://codereview.chromium.org/2282633002/diff/260001/media/mojo/services/mo... > File media/mojo/services/mojo_renderer_service.cc (right): > > https://codereview.chromium.org/2282633002/diff/260001/media/mojo/services/mo... > media/mojo/services/mojo_renderer_service.cc:32: MojoRendererService* > mojo_renderer_service = new MojoRendererService( > Nit: auto service = base::MakeUnique<MojoRendererService>(...); > > and then just std::move() below. Oh never mind, I see why you did it this way: you still need to reference the pointer. Incidentally, you probably don't need a setter: just mutate the field directly, since you're in the same class anyway. Might remove some boilerplate. > > https://codereview.chromium.org/2282633002/diff/260001/media/mojo/services/mo... > File media/mojo/services/mojo_renderer_service.h (right): > > https://codereview.chromium.org/2282633002/diff/260001/media/mojo/services/mo... > media/mojo/services/mojo_renderer_service.h:45: // NOTE: The MojoRendererService > will be uniquely owned by the returned > StrongBindingPtr is actually a WeakPtr. The created impl is actually owned by > the StrongBinding itself directly, while the StrongBindingImpl provides a safe > way to reference back to it.
gpu LGTM
Thank you for the comments everyone! Submitting to CQ. https://codereview.chromium.org/2282633002/diff/260001/media/mojo/services/mo... File media/mojo/services/mojo_renderer_service.cc (right): https://codereview.chromium.org/2282633002/diff/260001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.cc:32: MojoRendererService* mojo_renderer_service = new MojoRendererService( On 2016/10/05 22:37:12, dcheng wrote: > Nit: auto service = base::MakeUnique<MojoRendererService>(...); > > and then just std::move() below. I saw your follow up comment. I changed it for a moment because I thought it was slightly clearer to use a unique_ptr and to save a temp copy via '.get()', but then I ran into issues because MojoRendererService's ctor is now private, and base::MakeUnique couldn't access it (and friending it would have defeated the purpose of the private ctor). Good point about not needing the setter! https://codereview.chromium.org/2282633002/diff/260001/media/mojo/services/mo... File media/mojo/services/mojo_renderer_service.h (right): https://codereview.chromium.org/2282633002/diff/260001/media/mojo/services/mo... media/mojo/services/mojo_renderer_service.h:45: // NOTE: The MojoRendererService will be uniquely owned by the returned On 2016/10/05 22:37:12, dcheng wrote: > StrongBindingPtr is actually a WeakPtr. The created impl is actually owned by > the StrongBinding itself directly, while the StrongBindingImpl provides a safe > way to reference back to it. Clarified the comment.
The CQ bit was checked by tguilbert@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org, vmiura@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2282633002/#ps280001 (title: "Addressed last comments")
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 ========== Plumb StreamTexture to MediaPlayerRenderer The ScopedSurfaceRequestManager and ScopedSurfaceRequestConduit allow classes to register a ScopedJavaSurface request in the browser process, and to send SurfaceTexture from the GPU process to the browser process to fulfill those requests. This change adds the necessary plumbing to send a StreamTexture to a MediaPlayerRenderer living in the browser, using the manager/conduit pair. N.B: In order to prevent the MojoRendererService from taking a dependency on the MediaPlayerRenderer, the MojoRendererService initiates surface requests via a callback (given at construction time). BUG=627658 TEST=successfully exercised the new code path in a working 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 ========== Plumb StreamTexture to MediaPlayerRenderer The ScopedSurfaceRequestManager and ScopedSurfaceRequestConduit allow classes to register a ScopedJavaSurface request in the browser process, and to send SurfaceTexture from the GPU process to the browser process to fulfill those requests. This change adds the necessary plumbing to send a StreamTexture to a MediaPlayerRenderer living in the browser, using the manager/conduit pair. N.B: In order to prevent the MojoRendererService from taking a dependency on the MediaPlayerRenderer, the MojoRendererService initiates surface requests via a callback (given at construction time). BUG=627658 TEST=successfully exercised the new code path in a working 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 #14 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Plumb StreamTexture to MediaPlayerRenderer The ScopedSurfaceRequestManager and ScopedSurfaceRequestConduit allow classes to register a ScopedJavaSurface request in the browser process, and to send SurfaceTexture from the GPU process to the browser process to fulfill those requests. This change adds the necessary plumbing to send a StreamTexture to a MediaPlayerRenderer living in the browser, using the manager/conduit pair. N.B: In order to prevent the MojoRendererService from taking a dependency on the MediaPlayerRenderer, the MojoRendererService initiates surface requests via a callback (given at construction time). BUG=627658 TEST=successfully exercised the new code path in a working 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 ========== Plumb StreamTexture to MediaPlayerRenderer The ScopedSurfaceRequestManager and ScopedSurfaceRequestConduit allow classes to register a ScopedJavaSurface request in the browser process, and to send SurfaceTexture from the GPU process to the browser process to fulfill those requests. This change adds the necessary plumbing to send a StreamTexture to a MediaPlayerRenderer living in the browser, using the manager/conduit pair. N.B: In order to prevent the MojoRendererService from taking a dependency on the MediaPlayerRenderer, the MojoRendererService initiates surface requests via a callback (given at construction time). BUG=627658 TEST=successfully exercised the new code path in a working 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/ecb3febf6e08a15ceb4b9fa0af86e0a1591f315e Cr-Commit-Position: refs/heads/master@{#423393} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/ecb3febf6e08a15ceb4b9fa0af86e0a1591f315e Cr-Commit-Position: refs/heads/master@{#423393} |
