Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1873513003:
Add video-rendering to mojo media pipeline. (Closed)
|
Created:
4 years, 8 months ago by alokp Modified:
4 years, 7 months ago Reviewers:
sandersd (OOO until July 31), tguilbert, liberato (no reviews please), halliwell, DaleCurtis, Charlie Reis, xhwang CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd video-rendering to mojo media pipeline.
BUG=571155
Committed: https://crrev.com/16c8e68a20fc00d1c375af94770373d708ef3814
Cr-Commit-Position: refs/heads/master@{#394912}
Patch Set 1 #Patch Set 2 : added NaturalSizeChangedCB #Patch Set 3 : more plumbing #Patch Set 4 : minor cleanup #
Total comments: 38
Patch Set 5 : addressed comments #
Total comments: 3
Patch Set 6 : rebase #Patch Set 7 : uses VideoOverlayFactory in CmaRenderer #
Total comments: 4
Patch Set 8 : fixes component-build link error #Patch Set 9 : export VideoOverlayFactory #
Total comments: 3
Messages
Total messages: 43 (9 generated)
alokp@chromium.org changed reviewers: + dalecurtis@chromium.org, xhwang@chromium.org
This is not ready to commit. Sending it out to get feedback on the approach. The remote mojo media renderer needs a way to notify the media pipeline in the renderer process about video size changes so that it can: 1. Draw an updated video overlay frame 2. Notify blink so that it can do whatever is necessary (layout, etc.) The traditional pipeline gets this notification from VideoFrameCompositor. This patch simply moves it to the media::Renderer interface. I would appreciate feedback on this approach or if you have other ideas on how to implement this. FYI this is how CmaRenderer works right now. In fact the VideoOverlayFactory is simply copied from the chromecast code.
xhwang@chromium.org changed reviewers: + liberato@chromium.org
This CL looks good overall. I think it makes sense to plumb the NaturalSizeChangedCB in pipeline/renderer instead of having a shortcut between the video sink and WMPI. I left some comments for discussion. I also discussed this with liberato@ so adding him for comments as well. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/pipel... File media/base/pipeline.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/pipel... media/base/pipeline.h:58: // changes. Will this be called for the first frame? I feel calling it for the first frame makes sense because we have a new size). I don't remember what's the historical magic around the first frame though. +dalecurtis https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/pipel... File media/base/pipeline_status.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/pipel... media/base/pipeline_status.h:70: typedef base::Callback<void(const gfx::Size&)> NaturalSizeChangedCB; nit: This seems an odd place to declare this callback. Maybe we should have a separate file for this callback. Note that StatisticsCB doesn't quite belong to this file either. Or maybe we should rename this file. Think about it twice I do prefer the former. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/rende... File media/base/renderer.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/rende... media/base/renderer.h:38: // - |buffering_state_cb|: Executed when buffering state is changed. Add comment for the new callback https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/video... File media/base/video_renderer.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/video... media/base/video_renderer.h:45: // data or has enough data to continue playback. ditto about comment https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/blink/vide... File media/blink/video_frame_compositor.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/blink/vide... media/blink/video_frame_compositor.h:62: const base::Callback<void(bool)>& opacity_changed_cb); (not for this CL) Maybe we should move the opacity_changed_cb as well. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/blink/vide... media/blink/video_frame_compositor.h:124: // |opacity_changed_cb_| when the frame properties changes. nit: reformat https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/blink/vide... media/blink/video_frame_compositor.h:143: // These callbacks are executed on the compositor thread. nit: update this comment https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/blink/webm... File media/blink/webmediaplayer_impl.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/blink/webm... media/blink/webmediaplayer_impl.h:271: void OnNaturalSizeChanged(const gfx::Size& size); Interesting. I think your change makes sense: https://code.google.com/p/chromium/codesearch#search/&q=void.*%5C(gfx::Size&s... https://code.google.com/p/chromium/codesearch#search/&q=void.*%5C(const%5C%20... https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/inter... File media/mojo/interfaces/renderer.mojom (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/inter... media/mojo/interfaces/renderer.mojom:47: OnBufferingStateChange(BufferingState state); (not for this CL) This should be OnBufferingStateChange_d_ to be consistent with the rest of the pipeline. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... File media/mojo/services/mojo_renderer_factory.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/mojo_renderer_factory.h:13: #include "mojo/shell/public/interfaces/interface_provider.mojom.h" Why this change? Don't we prefer forward declaration? https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/mojo_renderer_factory.h:21: explicit MojoRendererFactory( nit: no explicit https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/mojo_renderer_factory.h:22: GpuVideoAcceleratorFactories* gpu_factories, You probably want GetGpuFactoriesCB as used in DefaultRendererFactory. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... File media/mojo/services/mojo_renderer_impl.cc (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/mojo_renderer_impl.cc:136: video_renderer_sink_->PaintFrameUsingOldRenderingPath( We have a comment that PaintFrameUsingOldRenderingPath() will go away. But this seems a legit use case. Maybe we should make this path official. +dalecurtis: WDYT? Did you have any plan to support overlay using the new RenderCallback path? https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... File media/mojo/services/video_overlay_factory.cc (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/video_overlay_factory.cc:16: VideoOverlayFactory::VideoOverlayFactory( Newbie question. Does this class work in all processes? https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/video_overlay_factory.cc:20: scoped_ptr<::media::GpuVideoAcceleratorFactories::ScopedGLContextLock> lock( We should keep using unique_ptr https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... File media/mojo/services/video_overlay_factory.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/video_overlay_factory.h:28: // All calls (including ctor/dtor) must be on media thread. Can you check this condition using thread checker or task runner? https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/video_overlay_factory.h:29: class VideoOverlayFactory { This has nothing to do with mojo and should probably be moved to a more common place like media/renderers. I imagine you could also use this to create an overlay in the render process without using anything related to mojo media. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/renderers/... File media/renderers/video_renderer_impl.cc (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/renderers/... media/renderers/video_renderer_impl.cc:248: // TODO(alokp): Fire NaturalSizeChangedCB if necessary. This could be related to this comment: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_r... There are multiple ways to do this: 1. As you proposed, move the natural size tracking logic to this class and fire the callback here. 2. Keep the logic in VideoFrameCompositor, and pass a NaturalSizeChangedCB in Start(), such that we only get notified when we are using the new rendering path (with Start()/Stop). We won't get notified when we are using the old rendering path. Thinking more on this, maybe (1) makes more sense...
Xiaohan: Thanks a lot for detailed suggestions. I am on vacation rest of this week. I will make the suggested changes next week when I am back in office. Dale/Frank: Could you please take a look at the overall approach in the meantime and let me know if this is in right direction. Thanks!
Dale: Do we need this path (or something similar) for the MediaPlayer work as well?
dalecurtis@chromium.org changed reviewers: + tguilbert@chromium.org
I'm not sure about what we're doing with MediaPlayer yet, +tguilbert in case he wants to share his thoughts on the investigation thus far for how to paint frames from Mojo. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/pipel... File media/base/pipeline.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/pipel... media/base/pipeline.h:58: // changes. On 2016/04/12 at 19:40:37, xhwang wrote: > Will this be called for the first frame? I feel calling it for the first frame makes sense because we have a new size). I don't remember what's the historical magic around the first frame though. > > +dalecurtis The VideoFrameCompositor wants this called for the first frame, without this Blink may not have the right size information IIRC.
I ran into an issue when mocking media::Pipeline. Now it exceeds 10 arguments supported by built-in MOCK_METHOD macros. I think we need to put these callbacks into a struct or create a RendererClient with those callbacks as member functions. I prefer RendererClient if it can be reconciled with mojo RendererClient. WDYT? https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/pipel... File media/base/pipeline_status.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/pipel... media/base/pipeline_status.h:70: typedef base::Callback<void(const gfx::Size&)> NaturalSizeChangedCB; On 2016/04/12 19:40:37, xhwang wrote: > nit: > > This seems an odd place to declare this callback. Maybe we should have a > separate file for this callback. Note that StatisticsCB doesn't quite belong to > this file either. > > Or maybe we should rename this file. > > Think about it twice I do prefer the former. I thought about this for some time and could not come up with a good file name. May be we should move all the callback definitions to renderer_callbacks.h or pipeline_callbacks.h. WDYT? https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/rende... File media/base/renderer.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/rende... media/base/renderer.h:38: // - |buffering_state_cb|: Executed when buffering state is changed. On 2016/04/12 19:40:37, xhwang wrote: > Add comment for the new callback Done. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/video... File media/base/video_renderer.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/base/video... media/base/video_renderer.h:45: // data or has enough data to continue playback. On 2016/04/12 19:40:37, xhwang wrote: > ditto about comment Done. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/blink/vide... File media/blink/video_frame_compositor.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/blink/vide... media/blink/video_frame_compositor.h:62: const base::Callback<void(bool)>& opacity_changed_cb); On 2016/04/12 19:40:37, xhwang wrote: > (not for this CL) Maybe we should move the opacity_changed_cb as well. yeah - I plan to do that in a followup patch. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/blink/vide... media/blink/video_frame_compositor.h:124: // |opacity_changed_cb_| when the frame properties changes. On 2016/04/12 19:40:37, xhwang wrote: > nit: reformat Done. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/blink/vide... media/blink/video_frame_compositor.h:143: // These callbacks are executed on the compositor thread. On 2016/04/12 19:40:37, xhwang wrote: > nit: update this comment Done. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/inter... File media/mojo/interfaces/renderer.mojom (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/inter... media/mojo/interfaces/renderer.mojom:47: OnBufferingStateChange(BufferingState state); On 2016/04/12 19:40:37, xhwang wrote: > (not for this CL) This should be OnBufferingStateChange_d_ to be consistent with > the rest of the pipeline. added TODO https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... File media/mojo/services/mojo_renderer_factory.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/mojo_renderer_factory.h:13: #include "mojo/shell/public/interfaces/interface_provider.mojom.h" On 2016/04/12 19:40:37, xhwang wrote: > Why this change? Don't we prefer forward declaration? restored. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/mojo_renderer_factory.h:21: explicit MojoRendererFactory( On 2016/04/12 19:40:37, xhwang wrote: > nit: no explicit Done. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/mojo_renderer_factory.h:22: GpuVideoAcceleratorFactories* gpu_factories, On 2016/04/12 19:40:37, xhwang wrote: > You probably want GetGpuFactoriesCB as used in DefaultRendererFactory. I am curious why this callback exists. Does it need to run on a particular thread? FWIW ContentRendererClient::CreateMediaRendererFactory does not use this callback - GpuVideoAcceleratorFactories is passed directly. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... File media/mojo/services/video_overlay_factory.cc (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/video_overlay_factory.cc:16: VideoOverlayFactory::VideoOverlayFactory( On 2016/04/12 19:40:37, xhwang wrote: > Newbie question. Does this class work in all processes? So far it has only been used in the renderer process, but technically can run in any process where GpuVideoAcceleratorFactories is available. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/video_overlay_factory.cc:20: scoped_ptr<::media::GpuVideoAcceleratorFactories::ScopedGLContextLock> lock( On 2016/04/12 19:40:37, xhwang wrote: > We should keep using unique_ptr Done. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... File media/mojo/services/video_overlay_factory.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/video_overlay_factory.h:28: // All calls (including ctor/dtor) must be on media thread. On 2016/04/12 19:40:37, xhwang wrote: > Can you check this condition using thread checker or task runner? Done. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/video_overlay_factory.h:29: class VideoOverlayFactory { On 2016/04/12 19:40:37, xhwang wrote: > This has nothing to do with mojo and should probably be moved to a more common > place like media/renderers. I imagine you could also use this to create an > overlay in the render process without using anything related to mojo media. Done. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/renderers/... File media/renderers/video_renderer_impl.cc (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/renderers/... media/renderers/video_renderer_impl.cc:248: // TODO(alokp): Fire NaturalSizeChangedCB if necessary. On 2016/04/12 19:40:38, xhwang wrote: > This could be related to this comment: > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_r... > > There are multiple ways to do this: > 1. As you proposed, move the natural size tracking logic to this class and fire > the callback here. > 2. Keep the logic in VideoFrameCompositor, and pass a NaturalSizeChangedCB in > Start(), such that we only get notified when we are using the new rendering path > (with Start()/Stop). We won't get notified when we are using the old rendering > path. > > Thinking more on this, maybe (1) makes more sense... Done.
I am not seeing a new PS to review :) For exceeding max number of parameters mocking media::Pipeline, do you mean the Pipeline::Start() call? We've been discussing whether we should replace the non-one-time callbacks with a Client interface. By the Client interface I mean a real interface (e.g. PipelineClient::OnNatualSizeChanged), not a class holding a collection of callbacks. Note that the |seek_cb| is a one-time callback and probably should not be on this client interface. Maybe we should rename this |seek_cb| to |start_cb| :) +sandersd since he refactored Pipeline recently.
On 2016/04/19 16:20:22, xhwang wrote: > I am not seeing a new PS to review :) > > For exceeding max number of parameters mocking media::Pipeline, do you mean the > Pipeline::Start() call? We've been discussing whether we should replace the > non-one-time callbacks with a Client interface. By the Client interface I mean a > real interface (e.g. PipelineClient::OnNatualSizeChanged), not a class holding a > collection of callbacks. > > Note that the |seek_cb| is a one-time callback and probably should not be on > this client interface. Maybe we should rename this |seek_cb| to |start_cb| :) > > +sandersd since he refactored Pipeline recently. Forgot to upload the latest patch. Done now. Yes - the 10 arguments limit is exceeded for Pipeline::Start(). I think we are on the same page. By RendererClient, I also meant a real class with member functions. But I would like to create RendererClient instead of PipelineClient. There is no need to duplicate those notifications in a PipelineClient interface.
On 2016/04/19 16:32:17, alokp wrote: > On 2016/04/19 16:20:22, xhwang wrote: > > I am not seeing a new PS to review :) > > > > For exceeding max number of parameters mocking media::Pipeline, do you mean > the > > Pipeline::Start() call? We've been discussing whether we should replace the > > non-one-time callbacks with a Client interface. By the Client interface I mean > a > > real interface (e.g. PipelineClient::OnNatualSizeChanged), not a class holding > a > > collection of callbacks. > > > > Note that the |seek_cb| is a one-time callback and probably should not be on > > this client interface. Maybe we should rename this |seek_cb| to |start_cb| :) > > > > +sandersd since he refactored Pipeline recently. > > Forgot to upload the latest patch. Done now. > > Yes - the 10 arguments limit is exceeded for Pipeline::Start(). I think we are > on the same page. By RendererClient, I also meant a real class with member > functions. But I would like to create RendererClient instead of PipelineClient. > There is no need to duplicate those notifications in a PipelineClient interface. xhwang@ is exactly correct about the plan. It's also a good place to eventually stick a task runner for posting the callbacks, if we decide to make that explicit. What's the difference between PipelineClient and RendererClient? If RendererClient has a subset of the same callbacks, then it could be a member of PipelineClient.
I am going to start defining a PipelineClient in a separate patch. We can look at RendererClient later.. may be when we hit the gmock limit for Renderer :p
xhwang@chromium.org changed reviewers: + sandersd@chromium.org
+sandersd: Please see the comment about GetGpuFactoriesCB. dalecurtis: Please see my old comment about whether it's recommended to use video_renderer_sink_->PaintFrameUsingOldRenderingPath() given the "old path" is planned to be removed. It seems to me what alokp@ is doing is a legit case. Maybe we should officially support "old path"? Otherwise this CL looks good to me. I'd like to have dalecurtis@ take a look as well. Also, I'll wait for your PipelineClient CL. https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... File media/mojo/services/mojo_renderer_factory.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/mojo_renderer_factory.h:22: GpuVideoAcceleratorFactories* gpu_factories, On 2016/04/19 00:16:07, alokp wrote: > On 2016/04/12 19:40:37, xhwang wrote: > > You probably want GetGpuFactoriesCB as used in DefaultRendererFactory. > > I am curious why this callback exists. Does it need to run on a particular > thread? FWIW ContentRendererClient::CreateMediaRendererFactory does not use this > callback - GpuVideoAcceleratorFactories is passed directly. Good point. It seems this is the reason: https://chromium.googlesource.com/chromium/src/+/0c2f582f1c68259ea17d2e95141f... Details: Instead of passing a GpuVideoAcceleratorFactories to the DefaultRendererFactory, and thus always using the same one for a given media player instance, pass a getter callback instead. Now DefaultRendererFactory can get the latest GpuVideoAcceleratorFactories instance each time a VideoDecoder is constructed. +sandersd for questions: - Shall we use the callback for all Renderer implementations? - Shall we have some comments that clients should always call GetGpuFactories() to get the latest one if possible? - Or can we actually hide this in RenderThreadImpl or RenderFrameImpl such that when we use the factory to do something, we'll always use the latest factory? https://chromiumcodereview.appspot.com/1873513003/diff/80001/content/renderer... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/1873513003/diff/80001/content/renderer... content/renderer/render_frame_impl.cc:22: #include "base/memory/ptr_util.h" nit: It would be nicer to keep rebase noise in a separate PS :) https://chromiumcodereview.appspot.com/1873513003/diff/80001/content/renderer... content/renderer/render_frame_impl.cc:2473: new media::MojoRendererFactory(render_thread->GetGpuFactories(), See my reply in the old PS. https://chromiumcodereview.appspot.com/1873513003/diff/80001/media/base/pipel... File media/base/pipeline_status.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/80001/media/base/pipel... media/base/pipeline_status.h:70: typedef base::Callback<void(const gfx::Size&)> NaturalSizeChangedCB; PipelineStatus should stay in pipeline_status.h. Maybe we can have another file like pipeline_callbacks.h to define these callbacks.
https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... File media/mojo/services/mojo_renderer_factory.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/mojo_renderer_factory.h:22: GpuVideoAcceleratorFactories* gpu_factories, > - Shall we use the callback for all Renderer implementations? Any Renderer that needs GpuFactories should use this callback. > - Shall we have some comments that clients should always call GetGpuFactories() > to get the latest one if possible? Sounds reasonable. The interaction isn't quite as simple as 'always use the latest', but as used by Renderers that's probably correct. The precise instructions would be to use the callback for each distinct 'session', where within one session we expect to always use the same GPU channel / command buffer. > - Or can we actually hide this in RenderThreadImpl or RenderFrameImpl such that > when we use the factory to do something, we'll always use the latest factory? I don't think that makes sense without a redesign of the whole structure (which is a good idea, but likely a lot of work). Notably, the DefaultRenderer only uses the GpuFactories for VDAs, and that will go away after VDAv2 replaces VDAv1 (could be a while to fully deprecate VDAv1 though).
On 2016/04/12 at 20:02:03, xhwang wrote: > Dale: Do we need this path (or something similar) for the MediaPlayer work as well? We don't need it per se, it would be fine to use the new path and just stop rendering after the new frame is sent to the compositor. The API is simpler though. The plan was to convert users like this to wait for the first Render() callback and then stop rendering if they only need to paint a hole frame.
https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... File media/mojo/services/mojo_renderer_factory.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/mojo_renderer_factory.h:22: GpuVideoAcceleratorFactories* gpu_factories, On 2016/04/20 18:50:06, sandersd wrote: > > - Shall we use the callback for all Renderer implementations? > > Any Renderer that needs GpuFactories should use this callback. I guess this is the easiest way to do for now. Can you file a bug so we also fix other Renderers? > > - Shall we have some comments that clients should always call > GetGpuFactories() > > to get the latest one if possible? > > Sounds reasonable. The interaction isn't quite as simple as 'always use the > latest', but as used by Renderers that's probably correct. > > The precise instructions would be to use the callback for each distinct > 'session', where within one session we expect to always use the same GPU channel > / command buffer. What's the definition of a "session"? Does it make sense to replace GetGpuFactories() with GetGpuFactoriesCreationCB() so that it won't be misused? Basically I think media/ shouldn't need to worry about details in GPU. > > - Or can we actually hide this in RenderThreadImpl or RenderFrameImpl such > that > > when we use the factory to do something, we'll always use the latest factory? > > I don't think that makes sense without a redesign of the whole structure (which > is a good idea, but likely a lot of work). > > Notably, the DefaultRenderer only uses the GpuFactories for VDAs, and that will > go away after VDAv2 replaces VDAv1 (could be a while to fully deprecate VDAv1 > though). Sadly acked.
https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... File media/mojo/services/mojo_renderer_factory.h (right): https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... media/mojo/services/mojo_renderer_factory.h:22: GpuVideoAcceleratorFactories* gpu_factories, On 2016/04/20 21:33:39, xhwang wrote: > > Any Renderer that needs GpuFactories should use this callback. > > I guess this is the easiest way to do for now. Can you file a bug so we also fix > other Renderers? Sorry, I was imprecise. I believe that in the general case for Renderers, they just need the result of this callback. RendererFactories, OTOH need to be more careful. This is based on the assumption that a single Renderer won't span multiple "sessions" as discussed below. > What's the definition of a "session"? > > Does it make sense to replace GetGpuFactories() with GetGpuFactoriesCreationCB() > so that it won't be misused? Basically I think media/ shouldn't need to worry > about details in GPU. As it turns out, media does need to worry about this, because GL contexts can be lost at any time, and a given factory stops working when that happens. It's especially bad for zero-copy VDAs, because in those cases we can't just re-upload the current VideoFrame into a new context. The precise details are these: - After a context loss, you can keep using a factory (and its command buffer) forever. It just doesn't *do* anything. - If you want to have a working factory/command buffer, you need to request a new one after context loss. - The current implementation only recovers from context loss by creating a new renderer (which happens when resuming from suspend). We don't recover from other situations yet, but some day we should.
On 2016/04/20 21:48:05, sandersd wrote: > https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... > File media/mojo/services/mojo_renderer_factory.h (right): > > https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... > media/mojo/services/mojo_renderer_factory.h:22: GpuVideoAcceleratorFactories* > gpu_factories, > On 2016/04/20 21:33:39, xhwang wrote: > > > Any Renderer that needs GpuFactories should use this callback. > > > > I guess this is the easiest way to do for now. Can you file a bug so we also > fix > > other Renderers? > > Sorry, I was imprecise. I believe that in the general case for Renderers, they > just need the result of this callback. RendererFactories, OTOH need to be more > careful. > > This is based on the assumption that a single Renderer won't span multiple > "sessions" as discussed below. > > > What's the definition of a "session"? > > > > Does it make sense to replace GetGpuFactories() with > GetGpuFactoriesCreationCB() > > so that it won't be misused? Basically I think media/ shouldn't need to worry > > about details in GPU. > > As it turns out, media does need to worry about this, because GL contexts can be > lost at any time, and a given factory stops working when that happens. It's > especially bad for zero-copy VDAs, because in those cases we can't just > re-upload the current VideoFrame into a new context. > > The precise details are these: > - After a context loss, you can keep using a factory (and its command buffer) > forever. It just doesn't *do* anything. > - If you want to have a working factory/command buffer, you need to request a > new one after context loss. > - The current implementation only recovers from context loss by creating a new > renderer (which happens when resuming from suspend). We don't recover from other > situations yet, but some day we should. If the renderer is re-created, then it is OK for a Renderer to hold a pointer to gpu factories, right? From your comments, it seems like this is only a problem for RendererFactories?
On 2016/04/20 21:59:01, alokp wrote: > On 2016/04/20 21:48:05, sandersd wrote: > > > https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... > > File media/mojo/services/mojo_renderer_factory.h (right): > > > > > https://chromiumcodereview.appspot.com/1873513003/diff/60001/media/mojo/servi... > > media/mojo/services/mojo_renderer_factory.h:22: GpuVideoAcceleratorFactories* > > gpu_factories, > > On 2016/04/20 21:33:39, xhwang wrote: > > > > Any Renderer that needs GpuFactories should use this callback. > > > > > > I guess this is the easiest way to do for now. Can you file a bug so we also > > fix > > > other Renderers? > > > > Sorry, I was imprecise. I believe that in the general case for Renderers, they > > just need the result of this callback. RendererFactories, OTOH need to be more > > careful. > > > > This is based on the assumption that a single Renderer won't span multiple > > "sessions" as discussed below. > > > > > What's the definition of a "session"? > > > > > > Does it make sense to replace GetGpuFactories() with > > GetGpuFactoriesCreationCB() > > > so that it won't be misused? Basically I think media/ shouldn't need to > worry > > > about details in GPU. > > > > As it turns out, media does need to worry about this, because GL contexts can > be > > lost at any time, and a given factory stops working when that happens. It's > > especially bad for zero-copy VDAs, because in those cases we can't just > > re-upload the current VideoFrame into a new context. > > > > The precise details are these: > > - After a context loss, you can keep using a factory (and its command > buffer) > > forever. It just doesn't *do* anything. > > - If you want to have a working factory/command buffer, you need to request > a > > new one after context loss. > > - The current implementation only recovers from context loss by creating a > new > > renderer (which happens when resuming from suspend). We don't recover from > other > > situations yet, but some day we should. > > If the renderer is re-created, then it is OK for a Renderer to hold a pointer to > gpu factories, right? From your comments, it seems like this is only a problem > for RendererFactories? Conveniently/horrifyingly, it's always safe to hold a pointer, because GpuFactories are never destructed. They just might not be hooked up to the GPU process anymore. But yes, with the currently limited form of context loss recovery, Renderers shouldn't need to know about this; they can just have a raw pointer to a GpuFactory. I expect that this will remain true in the future; we will probably implement full context loss recovery by creating a new Renderer when we detect it.
Most of the changes in the original patch have already landed separately: https://codereview.chromium.org/1904793002 https://codereview.chromium.org/1955843002 https://codereview.chromium.org/1978973002 This patch now only creates overlay frames in response to natural-size-change notification from remote renderer. PTAL.
https://codereview.chromium.org/1873513003/diff/120001/media/renderers/video_... File media/renderers/video_overlay_factory.cc (right): https://codereview.chromium.org/1873513003/diff/120001/media/renderers/video_... media/renderers/video_overlay_factory.cc:77: frame->metadata()->SetBoolean(VideoFrameMetadata::ALLOW_OVERLAY, true); Are the side effects from scheduling the overlay important in the paths that use this? (Because if not, there are likely simpler ways.)
https://codereview.chromium.org/1873513003/diff/120001/media/renderers/video_... File media/renderers/video_overlay_factory.cc (right): https://codereview.chromium.org/1873513003/diff/120001/media/renderers/video_... media/renderers/video_overlay_factory.cc:77: frame->metadata()->SetBoolean(VideoFrameMetadata::ALLOW_OVERLAY, true); On 2016/05/18 00:47:47, sandersd wrote: > Are the side effects from scheduling the overlay important in the paths that use > this? > > (Because if not, there are likely simpler ways.) This is how chromecast renderer works currently. During Ozone CheckOverlaySupport, we mark the overlay as handled and resize the video plane: https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/... https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/media/b... I am open to simpler ways :)
lgtm https://codereview.chromium.org/1873513003/diff/120001/media/renderers/video_... File media/renderers/video_overlay_factory.cc (right): https://codereview.chromium.org/1873513003/diff/120001/media/renderers/video_... media/renderers/video_overlay_factory.cc:77: frame->metadata()->SetBoolean(VideoFrameMetadata::ALLOW_OVERLAY, true); On 2016/05/18 04:15:02, alokp wrote: > On 2016/05/18 00:47:47, sandersd wrote: > > Are the side effects from scheduling the overlay important in the paths that > use > > this? > > > > (Because if not, there are likely simpler ways.) > > This is how chromecast renderer works currently. During Ozone > CheckOverlaySupport, we mark the overlay as handled and resize the video plane: > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/... > > https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/media/b... > > I am open to simpler ways :) Okay, I see. This ends up being the same as our BrowserSurfaceViewManager getting called in OnNaturalSizeChanged(), except without a custom IPC. Perhaps after the Android path stabilizes we can see about unforking the underlay paths.
https://codereview.chromium.org/1873513003/diff/120001/media/renderers/video_... File media/renderers/video_overlay_factory.cc (right): https://codereview.chromium.org/1873513003/diff/120001/media/renderers/video_... media/renderers/video_overlay_factory.cc:77: frame->metadata()->SetBoolean(VideoFrameMetadata::ALLOW_OVERLAY, true); On 2016/05/18 17:57:08, sandersd wrote: > On 2016/05/18 04:15:02, alokp wrote: > > On 2016/05/18 00:47:47, sandersd wrote: > > > Are the side effects from scheduling the overlay important in the paths that > > use > > > this? > > > > > > (Because if not, there are likely simpler ways.) > > > > This is how chromecast renderer works currently. During Ozone > > CheckOverlaySupport, we mark the overlay as handled and resize the video > plane: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/... > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/media/b... > > > > I am open to simpler ways :) > > Okay, I see. This ends up being the same as our BrowserSurfaceViewManager > getting called in OnNaturalSizeChanged(), except without a custom IPC. > > Perhaps after the Android path stabilizes we can see about unforking the > underlay paths. Acknowledged.
alokp@chromium.org changed reviewers: + creis@chromium.org
creis: content/
alokp@chromium.org changed reviewers: + halliwell@chromium.org
Luke: FYI - chromecast HoleFrameFactory is moved into media as VideoOverlayFactory. I have updated CmaRenderer to use the new class. https://chromiumcodereview.appspot.com/1873513003/diff/150001/media/test/BUIL... File media/test/BUILD.gn (left): https://chromiumcodereview.appspot.com/1873513003/diff/150001/media/test/BUIL... media/test/BUILD.gn:49: "//media/mojo/interfaces", Xiaohan: AFAICT pipeline_integration_tests does not have any dependency on media/mojo. Was this intentional? I have also removed the deps already included in pipeline_integration_test_base and verified that gn-check still passes.
I can't really tell what's going on here in render_frame_impl.cc, but it looks pretty similar to line 2525, so rubber stamp LGTM for content/.
https://codereview.chromium.org/1873513003/diff/150001/media/renderers/video_... File media/renderers/video_overlay_factory.h (right): https://codereview.chromium.org/1873513003/diff/150001/media/renderers/video_... media/renderers/video_overlay_factory.h:27: // This class must be used on GpuVideoAcceleratorFactories::GetTaskRunner(). This comment seems a little incomplete now this class is in media/. Should probably clarify that the frames produced are dummy frames for the sole purpose of carrying the overlay flag through to the compositor, to be replaced there. This assumes that the real video frame is rendered externally. e.g. not to be confused with the overlay frames produced by GpuVideoDecoder.
lgtm dalecurtis: Could you please also take a look at the changes related to VideoRendererSink and VideoOverlayFactory? https://chromiumcodereview.appspot.com/1873513003/diff/150001/media/test/BUIL... File media/test/BUILD.gn (left): https://chromiumcodereview.appspot.com/1873513003/diff/150001/media/test/BUIL... media/test/BUILD.gn:49: "//media/mojo/interfaces", On 2016/05/18 20:53:24, alokp wrote: > Xiaohan: AFAICT pipeline_integration_tests does not have any dependency on > media/mojo. Was this intentional? > > I have also removed the deps already included in pipeline_integration_test_base > and verified that gn-check still passes. This target doesn't define MOJO_RENDERER and doesn't test mojo stuff. So this change lg. Thanks!
What changes related to VideoRendererSink? I don't see any.
OIC now. Looking.
Eh, this isn't really ideal, but is fine for now since Chromecast is already the only user of the old path. Instead of using the old path for painting you should return the hole frame after calling StartSink(), waiting for a Render() call, and then calling StopSink().
Although I'm beginning to think maybe we should just have a single frame paint method like this since VRI has some junk in it to paint a single frame too. So given that, for now this is fine. I'll clean it up in a week or so if I go that route.
The CQ bit was checked by alokp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/1873513003/#ps150001 (title: "export VideoOverlayFactory")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1873513003/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1873513003/150001
Message was sent while issue was closed.
Committed patchset #9 (id:150001)
Message was sent while issue was closed.
Description was changed from ========== Add video-rendering to mojo media pipeline. BUG=571155 ========== to ========== Add video-rendering to mojo media pipeline. BUG=571155 Committed: https://crrev.com/16c8e68a20fc00d1c375af94770373d708ef3814 Cr-Commit-Position: refs/heads/master@{#394912} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/16c8e68a20fc00d1c375af94770373d708ef3814 Cr-Commit-Position: refs/heads/master@{#394912} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
