Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521133002:
media: Add MediaInterfaceProxy (Closed)
|
Created:
4 years, 1 month ago by xhwang Modified:
4 years ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, feature-media-reviews_chromium.org, sandersd (OOO until July 31), tguilbert Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Add MediaInterfaceProxy
The current structure make it impossible to support hosting different
media services in different processes. For example, on desktop we may
want to host a video decoder service in the GPU process, but host a CDM
service in a utility process. For another example, on Android, we want
to host the audio decoder and CDM service in the GPU process, but host
a Renderer service in the browser process.
This CL adds a MediaInterfaceProxy which will serve as a proxy (and
central place) to dispatch media interface creation calls and decide
where to forward the interface request to support the above use cases.
In this CL, MediaInterfaceProxy will always forward calls to the
existing media service we have. In future CLs, we'll add more logic to
support the more complicated use cases.
This also moves a lot of media logic into a helper class, which helps
make RenderFrameHostImpl cleaner.
BUG=664364
TEST=Maually tested.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/aa0bf6dcfa0c1e3e788d381cebee7e012a3a5a6e
Cr-Commit-Position: refs/heads/master@{#436119}
Patch Set 1 #Patch Set 2 : rebase only #Patch Set 3 : rebase only #
Total comments: 6
Patch Set 4 : rebase only #Patch Set 5 : comments addressed #
Messages
Total messages: 42 (24 generated)
Description was changed from ========== media: Add MediaInterfaceProxy The current structure make it impossible to support hosting different media services in different processes. For example, on desktop we may want to host a video decoder service in the GPU process, but host a CDM service in a utility process. For another example, on Android, we want to host the audio decoder and CDM service in the GPU process, but host a Renderer service in the browser process. This CL adds a MediaInterfaceProxy which will serve as a proxy (and central place) to dispatch media interface creation calls and decide where to forward the interface request to support the above use cases. In this CL, MediaInterfaceProxy will always forward calls to the existing media service we have. In future CLs, we'll add more logic to support the more complicated use cases. This also moves a lot of media logic into a helper class, which helps make RenderFrameHostImpl cleaner. BUG=664364 TEST=Maually tested. ========== to ========== media: Add MediaInterfaceProxy The current structure make it impossible to support hosting different media services in different processes. For example, on desktop we may want to host a video decoder service in the GPU process, but host a CDM service in a utility process. For another example, on Android, we want to host the audio decoder and CDM service in the GPU process, but host a Renderer service in the browser process. This CL adds a MediaInterfaceProxy which will serve as a proxy (and central place) to dispatch media interface creation calls and decide where to forward the interface request to support the above use cases. In this CL, MediaInterfaceProxy will always forward calls to the existing media service we have. In future CLs, we'll add more logic to support the more complicated use cases. This also moves a lot of media logic into a helper class, which helps make RenderFrameHostImpl cleaner. BUG=664364 TEST=Maually tested. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by xhwang@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...)
rebase only
The CQ bit was checked by xhwang@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_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by xhwang@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...
rebase only
The CQ bit was checked by xhwang@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.
xhwang@chromium.org changed reviewers: + alokp@chromium.org
alokp: PTAL sandersd / tguilbert: FYI
On 2016/11/30 17:42:16, xhwang wrote: > alokp: PTAL > sandersd / tguilbert: FYI This patch looks fine since it moves most of the ugliness out of RenderFrameHostImpl. That said I am not sure if MediaInterfaceProxy should be responsible for dispatching requests to the correct process. To me, this defeats the purpose of using mojo. The client should not have to go via a proxy to request interfaces. It should be able to simply connect to the media service and use it. It should be the responsibility of media service implementation to transparently fulfill the request. For interfaces hosted remotely in gpu process for example, would it possible to make this work by simply forwarding the pipe handle from the media service to service:content_gpu? Just my 2 cents. This patch lgtm.
On 2016/12/01 01:23:35, alokp wrote: > On 2016/11/30 17:42:16, xhwang wrote: > > alokp: PTAL > > sandersd / tguilbert: FYI > > This patch looks fine since it moves most of the ugliness out of > RenderFrameHostImpl. > > That said I am not sure if MediaInterfaceProxy should be responsible for > dispatching requests to the correct process. To me, this defeats the purpose of > using mojo. The client should not have to go via a proxy to request interfaces. > It should be able to simply connect to the media service and use it. It should > be the responsibility of media service implementation to transparently fulfill > the request. For interfaces hosted remotely in gpu process for example, would it > possible to make this work by simply forwarding the pipe handle from the media > service to service:content_gpu? > > Just my 2 cents. This patch lgtm. I am not sure I follow your suggestion. The true client, renderer process, has no concept of MediaService, it basically request a media::mojom::InterfaceFactory at the RenderFrame level. It's the RenderFrameHostImpl's implementation detail that the InterfaceFactory is provided by a MediaService running in-process or out-of-process. This CL is only changing the impl detail and will not change how the "client" connects to media services. The ultimate goal is to support creating media interface implemenations running in different processes, e.g. CDM running in utility process and VideoDecoder running in GPU process. I am not sure how your example would solve this. Someone has to make the decision where to forward the media interface requests. I feel it's more natural to have this happening in the browser process, then in some other process, e.g. GPU. In that case, how we can we handle multiple service instances?
xhwang@chromium.org changed reviewers: + nasko@chromium.org
nasko: Please content OWNERS review. You can treat this CL as a refactor/clean-up CL, which by itself is an improvement IMHO. It also provides more flexibility how we connect to media services, which I'll enable in future CLs.
On 2016/12/01 06:17:39, xhwang wrote: > On 2016/12/01 01:23:35, alokp wrote: > > On 2016/11/30 17:42:16, xhwang wrote: > > > alokp: PTAL > > > sandersd / tguilbert: FYI > > > > This patch looks fine since it moves most of the ugliness out of > > RenderFrameHostImpl. > > > > That said I am not sure if MediaInterfaceProxy should be responsible for > > dispatching requests to the correct process. To me, this defeats the purpose > of > > using mojo. The client should not have to go via a proxy to request > interfaces. > > It should be able to simply connect to the media service and use it. It should > > be the responsibility of media service implementation to transparently fulfill > > the request. For interfaces hosted remotely in gpu process for example, would > it > > possible to make this work by simply forwarding the pipe handle from the media > > service to service:content_gpu? > > > > Just my 2 cents. This patch lgtm. > > I am not sure I follow your suggestion. The true client, renderer process, has > no concept of MediaService, it basically request a > media::mojom::InterfaceFactory at the RenderFrame level. It's the > RenderFrameHostImpl's implementation detail that the InterfaceFactory is > provided by a MediaService running in-process or out-of-process. This CL is only > changing the impl detail and will not change how the "client" connects to media > services. Renderer process is just one of the clients of media service. For chromecast we have a few more that have nothing to do with the browser. What if renderer process used the media service directly instead of via RenderFrame registry? > The ultimate goal is to support creating media interface implemenations running > in different processes, e.g. CDM running in utility process and VideoDecoder > running in GPU process. I am not sure how your example would solve this. Someone > has to make the decision where to forward the media interface requests. I feel > it's more natural to have this happening in the browser process, then in some > other process, e.g. GPU. I am not saying that we should not run media service implementations in different process. Instead the entity that forwards the media interface requests to appropriate process should be the media service instead of this MediaInterfaceProxy that only one client - renderer process - can use. > In that case, how we can we handle multiple service instances? I do not understand this statement. Do you mean multiple media-service instances? That can only happen when multiple user profiles is used, right? Even so, what is the problem?
On 2016/12/01 13:20:57, alokp wrote: > On 2016/12/01 06:17:39, xhwang wrote: > > On 2016/12/01 01:23:35, alokp wrote: > > > On 2016/11/30 17:42:16, xhwang wrote: > > > > alokp: PTAL > > > > sandersd / tguilbert: FYI > > > > > > This patch looks fine since it moves most of the ugliness out of > > > RenderFrameHostImpl. > > > > > > That said I am not sure if MediaInterfaceProxy should be responsible for > > > dispatching requests to the correct process. To me, this defeats the purpose > > of > > > using mojo. The client should not have to go via a proxy to request > > interfaces. > > > It should be able to simply connect to the media service and use it. It > should > > > be the responsibility of media service implementation to transparently > fulfill > > > the request. For interfaces hosted remotely in gpu process for example, > would > > it > > > possible to make this work by simply forwarding the pipe handle from the > media > > > service to service:content_gpu? > > > > > > Just my 2 cents. This patch lgtm. > > > > I am not sure I follow your suggestion. The true client, renderer process, has > > no concept of MediaService, it basically request a > > media::mojom::InterfaceFactory at the RenderFrame level. It's the > > RenderFrameHostImpl's implementation detail that the InterfaceFactory is > > provided by a MediaService running in-process or out-of-process. This CL is > only > > changing the impl detail and will not change how the "client" connects to > media > > services. > > Renderer process is just one of the clients of media service. For chromecast we > have a few more that have nothing to do with the browser. What if renderer > process used the media service directly instead of via RenderFrame registry? The proposed change doesn't touch the MediaService itself, what is working now will still work. > > The ultimate goal is to support creating media interface implemenations > running > > in different processes, e.g. CDM running in utility process and VideoDecoder > > running in GPU process. I am not sure how your example would solve this. > Someone > > has to make the decision where to forward the media interface requests. I feel > > it's more natural to have this happening in the browser process, then in some > > other process, e.g. GPU. > > I am not saying that we should not run media service implementations in > different process. Instead the entity that forwards the media interface requests > to appropriate process should be the media service instead of this > MediaInterfaceProxy that only one client - renderer process - can use. Typically we deal with multi-process-ness in the content/ layer. Also, I'd like to keep MediaService simple. Does ChromeCast need this now? If so, we can talk and see how we can make it work for all cases. Otherwise, I'd like to stay with simpler design to follow the YAGNI principle. > > In that case, how we can we handle multiple service instances? > > I do not understand this statement. Do you mean multiple media-service > instances? That can only happen when multiple user profiles is used, right? Even > so, what is the problem? oops, this was supposed to be deleted :)
> Typically we deal with multi-process-ness in the content/ layer. Also, I'd like > to keep MediaService simple. Does ChromeCast need this now? If so, we can talk > and see how we can make it work for all cases. Otherwise, I'd like to stay with > simpler design to follow the YAGNI principle. As I said this patch is OK for now. My concern is about future plans for MediaInterfaceProxy. We can discuss that when we get there.
On 2016/12/01 18:26:58, alokp wrote: > > Typically we deal with multi-process-ness in the content/ layer. Also, I'd > like > > to keep MediaService simple. Does ChromeCast need this now? If so, we can > talk > > and see how we can make it work for all cases. Otherwise, I'd like to stay > with > > simpler design to follow the YAGNI principle. > > As I said this patch is OK for now. My concern is about future plans for > MediaInterfaceProxy. We can discuss that when we get there. Sure, thanks! :) nasko: PTAL
nasko: Kindly ping!
LGTM with nits https://codereview.chromium.org/2521133002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2521133002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:819: nit: Unneeded empty line. https://codereview.chromium.org/2521133002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:1073: // Hosts media::mojom::InterfaceFactory for the render frame and forwards nit: RenderFrame https://codereview.chromium.org/2521133002/diff/40001/content/browser/media/m... File content/browser/media/media_interface_proxy.h (right): https://codereview.chromium.org/2521133002/diff/40001/content/browser/media/m... content/browser/media/media_interface_proxy.h:21: // This implements the InterfaceFactory interface for RenderFrameHostImpl. Upon nit: This class nit: prepend media::mojom:: to InterfaceFactory, so it is not ambiguous which one is meant.
rebase only
comments addressed
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2521133002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2521133002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:819: On 2016/12/02 22:18:09, nasko wrote: > nit: Unneeded empty line. Done. https://codereview.chromium.org/2521133002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:1073: // Hosts media::mojom::InterfaceFactory for the render frame and forwards On 2016/12/02 22:18:09, nasko wrote: > nit: RenderFrame Done. https://codereview.chromium.org/2521133002/diff/40001/content/browser/media/m... File content/browser/media/media_interface_proxy.h (right): https://codereview.chromium.org/2521133002/diff/40001/content/browser/media/m... content/browser/media/media_interface_proxy.h:21: // This implements the InterfaceFactory interface for RenderFrameHostImpl. Upon On 2016/12/02 22:18:09, nasko wrote: > nit: This class > nit: prepend media::mojom:: to InterfaceFactory, so it is not ambiguous which > one is meant. Done.
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 xhwang@chromium.org
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org, alokp@chromium.org Link to the patchset: https://codereview.chromium.org/2521133002/#ps80001 (title: "comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480722386021670,
"parent_rev": "39e0637cdb14176c55c986fc53e8363bc4b10ea2", "commit_rev":
"d66031a5de1a8715b7ffddae2a77f146533e17cd"}
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== media: Add MediaInterfaceProxy The current structure make it impossible to support hosting different media services in different processes. For example, on desktop we may want to host a video decoder service in the GPU process, but host a CDM service in a utility process. For another example, on Android, we want to host the audio decoder and CDM service in the GPU process, but host a Renderer service in the browser process. This CL adds a MediaInterfaceProxy which will serve as a proxy (and central place) to dispatch media interface creation calls and decide where to forward the interface request to support the above use cases. In this CL, MediaInterfaceProxy will always forward calls to the existing media service we have. In future CLs, we'll add more logic to support the more complicated use cases. This also moves a lot of media logic into a helper class, which helps make RenderFrameHostImpl cleaner. BUG=664364 TEST=Maually tested. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== media: Add MediaInterfaceProxy The current structure make it impossible to support hosting different media services in different processes. For example, on desktop we may want to host a video decoder service in the GPU process, but host a CDM service in a utility process. For another example, on Android, we want to host the audio decoder and CDM service in the GPU process, but host a Renderer service in the browser process. This CL adds a MediaInterfaceProxy which will serve as a proxy (and central place) to dispatch media interface creation calls and decide where to forward the interface request to support the above use cases. In this CL, MediaInterfaceProxy will always forward calls to the existing media service we have. In future CLs, we'll add more logic to support the more complicated use cases. This also moves a lot of media logic into a helper class, which helps make RenderFrameHostImpl cleaner. BUG=664364 TEST=Maually tested. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/aa0bf6dcfa0c1e3e788d381cebee7e012a3a5a6e Cr-Commit-Position: refs/heads/master@{#436119} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/aa0bf6dcfa0c1e3e788d381cebee7e012a3a5a6e Cr-Commit-Position: refs/heads/master@{#436119} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
