Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2684103005:
Allow media track switching. (Closed)
|
Created:
3 years, 10 months ago by servolk Modified:
3 years, 8 months ago CC:
apacible+watch_chromium.org, chromium-reviews, erickung+watch_chromium.org, feature-media-reviews_chromium.org, miu+watch_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow media track switching.
This CL allows Audio and VideoRenderer to be reinitialized in the
flushed/stopped states and allows switching between different audio
and video tracks during playback (but only a single track of each type
may be active at a time).
BUG=487288
Review-Url: https://codereview.chromium.org/2684103005
Cr-Commit-Position: refs/heads/master@{#463658}
Committed: https://chromium.googlesource.com/chromium/src/+/16e8bdf82977fa9734263248666543e204a930a4
Patch Set 1 #Patch Set 2 : fix initial track selection #Patch Set 3 : Add tests #Patch Set 4 : Rebase + more tests and fixes #Patch Set 5 : rebase #Patch Set 6 : Log a warning when ignoring enabled audio tracks after the first one #Patch Set 7 : rebase #Patch Set 8 : rebase #Patch Set 9 : Restore CreateRenderer in pipeline_integration_test.cc #
Total comments: 8
Patch Set 10 : Allow mojo media interfaces to be obtained from any thread #Patch Set 11 : CR feedback #
Total comments: 10
Patch Set 12 : rebase #Patch Set 13 : Create MediaInterfaceProvider weak ptr on the main thread #Patch Set 14 : Added a LayoutTest for media track switching #
Total comments: 32
Patch Set 15 : CR feedback #
Total comments: 20
Patch Set 16 : CR feedback #Patch Set 17 : Postpone Flush while handling stream status changes #Patch Set 18 : Added a note in GpuMemoryBufferVideoFramePool about memory buffers being refcounted #Patch Set 19 : rebase / resolve conflict #
Total comments: 4
Patch Set 20 : Updated GpuMemoryBufferVideoFramePool comment #Patch Set 21 : Updated GpuMemoryBufferVideoFramePool comment #
Total comments: 25
Patch Set 22 : More unit tests + CR feedback #Patch Set 23 : Updated log message #
Total comments: 8
Patch Set 24 : Fixed comment #Patch Set 25 : Rearrange methods to match declaration order #Patch Set 26 : rebase #Patch Set 27 : Properly handle track status changes in FLUSHED state #
Total comments: 11
Patch Set 28 : rebase #Patch Set 29 : Added comments in RestartA/VRenderer #Patch Set 30 : CR feedback #Patch Set 31 : Fixed comments #Messages
Total messages: 192 (124 generated)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Rebase + audio track switching
==========
to
==========
Allow media track switching.
This CL allows Audio and VideoRenderer to be reinitialized in the
flushed/stopped states and allows switching between different audio
and video tracks during playback (but only a single track of each type
may be active at a time).
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by servolk@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by servolk@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by servolk@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by servolk@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
servolk@chromium.org changed reviewers: + dalecurtis@chromium.org, xhwang@chromium.org
The CQ bit was checked by servolk@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by servolk@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Allow media track switching.
This CL allows Audio and VideoRenderer to be reinitialized in the
flushed/stopped states and allows switching between different audio
and video tracks during playback (but only a single track of each type
may be active at a time).
==========
to
==========
Allow media track switching.
This CL allows Audio and VideoRenderer to be reinitialized in the
flushed/stopped states and allows switching between different audio
and video tracks during playback (but only a single track of each type
may be active at a time).
BUG=487288
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Do you have a demo page for this functionality? I'd like to get a better sense for what it feels and sounds like.
On 2017/03/16 22:25:15, DaleCurtis wrote: > Do you have a demo page for this functionality? I'd like to get a better sense > for what it feels and sounds like. I'm using https://storage.googleapis.com/servolk/red-green3.mp4 to test this CL end-to-end. That file has two audio tracks and two video tracks. The audio tracks are sine wave tones at 300Hz and 500Hz, the video tracks are just full-frame solid colors. The first video track is red color with 640x320 coded frame size, the second one is green color with 320x240 coded frame size. If you build chromium with this patch, just open a new browser instance and navigate to the URL above. That will start playing the first video track and the first audio track automatically. Then in order to switch audio tracks you can do something like this through dev console (keep in mind we are still limited to a single audio track for now): document.getElementsByTagName('video')[0].audioTracks[0].enabled = false; document.getElementsByTagName('video')[0].audioTracks[1].enabled = true; And to switch video tracks you can do something like this (selecting video track 1 automatically deselects video track 0): document.getElementsByTagName('video')[0].videoTracks[1].selected = true
On 2017/03/16 22:35:57, servolk wrote: > On 2017/03/16 22:25:15, DaleCurtis wrote: > > Do you have a demo page for this functionality? I'd like to get a better sense > > for what it feels and sounds like. > > I'm using https://storage.googleapis.com/servolk/red-green3.mp4 to test this CL > end-to-end. That file has two audio tracks and two video tracks. The audio > tracks are sine wave tones at 300Hz and 500Hz, the video tracks are just > full-frame solid colors. The first video track is red color with 640x320 coded > frame size, the second one is green color with 320x240 coded frame size. If you > build chromium with this patch, just open a new browser instance and navigate to > the URL above. That will start playing the first video track and the first audio > track automatically. > Then in order to switch audio tracks you can do something like this through dev > console (keep in mind we are still limited to a single audio track for now): > > document.getElementsByTagName('video')[0].audioTracks[0].enabled = false; > document.getElementsByTagName('video')[0].audioTracks[1].enabled = true; > > And to switch video tracks you can do something like this (selecting video track > 1 automatically deselects video track 0): > > document.getElementsByTagName('video')[0].videoTracks[1].selected = true Oh, and btw, you'll need to run Chrome with --enable-experimental-web-platform-features command-line param in order to be able to test this.
On 2017/03/16 22:44:58, servolk wrote: > On 2017/03/16 22:35:57, servolk wrote: > > On 2017/03/16 22:25:15, DaleCurtis wrote: > > > Do you have a demo page for this functionality? I'd like to get a better > sense > > > for what it feels and sounds like. > > > > I'm using https://storage.googleapis.com/servolk/red-green3.mp4 to test this > CL > > end-to-end. That file has two audio tracks and two video tracks. The audio > > tracks are sine wave tones at 300Hz and 500Hz, the video tracks are just > > full-frame solid colors. The first video track is red color with 640x320 coded > > frame size, the second one is green color with 320x240 coded frame size. If > you > > build chromium with this patch, just open a new browser instance and navigate > to > > the URL above. That will start playing the first video track and the first > audio > > track automatically. > > Then in order to switch audio tracks you can do something like this through > dev > > console (keep in mind we are still limited to a single audio track for now): > > > > document.getElementsByTagName('video')[0].audioTracks[0].enabled = false; > > document.getElementsByTagName('video')[0].audioTracks[1].enabled = true; > > > > And to switch video tracks you can do something like this (selecting video > track > > 1 automatically deselects video track 0): > > > > document.getElementsByTagName('video')[0].videoTracks[1].selected = true > > Oh, and btw, you'll need to run Chrome with > --enable-experimental-web-platform-features command-line param in order to be > able to test this. Sorry, another small correction https://storage.googleapis.com/servolk/red-green3.mp4 contains two video tracks, but only one audio track. So please use https://storage.googleapis.com/servolk/red-green4.mp4 - this one has two video and two audio tracks. I've tried updating the red-green3.mp4, but somehow I'm still getting the old (cached?) version from somewhere. But https://storage.googleapis.com/servolk/red-green4.mp works fine.
On 2017/03/16 23:29:20, servolk wrote: > On 2017/03/16 22:44:58, servolk wrote: > > On 2017/03/16 22:35:57, servolk wrote: > > > On 2017/03/16 22:25:15, DaleCurtis wrote: > > > > Do you have a demo page for this functionality? I'd like to get a better > > sense > > > > for what it feels and sounds like. > > > > > > I'm using https://storage.googleapis.com/servolk/red-green3.mp4 to test this > > CL > > > end-to-end. That file has two audio tracks and two video tracks. The audio > > > tracks are sine wave tones at 300Hz and 500Hz, the video tracks are just > > > full-frame solid colors. The first video track is red color with 640x320 > coded > > > frame size, the second one is green color with 320x240 coded frame size. If > > you > > > build chromium with this patch, just open a new browser instance and > navigate > > to > > > the URL above. That will start playing the first video track and the first > > audio > > > track automatically. > > > Then in order to switch audio tracks you can do something like this through > > dev > > > console (keep in mind we are still limited to a single audio track for now): > > > > > > document.getElementsByTagName('video')[0].audioTracks[0].enabled = false; > > > document.getElementsByTagName('video')[0].audioTracks[1].enabled = true; > > > > > > And to switch video tracks you can do something like this (selecting video > > track > > > 1 automatically deselects video track 0): > > > > > > document.getElementsByTagName('video')[0].videoTracks[1].selected = true > > > > Oh, and btw, you'll need to run Chrome with > > --enable-experimental-web-platform-features command-line param in order to be > > able to test this. > > Sorry, another small correction > https://storage.googleapis.com/servolk/red-green3.mp4 contains two video tracks, > but only one audio track. So please use > https://storage.googleapis.com/servolk/red-green4.mp4 - this one has two video > and two audio tracks. I've tried updating the red-green3.mp4, but somehow I'm > still getting the old (cached?) version from somewhere. But > https://storage.googleapis.com/servolk/red-green4.mp works fine. ping
The CQ bit was checked by servolk@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...
Sorry don't have time to look at this until next week.
On 2017/03/17 22:43:24, DaleCurtis wrote: > Sorry don't have time to look at this until next week. Shall we have a meeting talking about the design first before we get into implementation review? :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/03/17 23:42:36, xhwang_slow wrote: > On 2017/03/17 22:43:24, DaleCurtis wrote: > > Sorry don't have time to look at this until next week. > > Shall we have a meeting talking about the design first before we get into > implementation review? :) Sure, I can schedule something next week. But if you get a chance, please take a look at this CL as well, it's a prototype, but it demonstrates the option 1 described in the go/media-track-switching doc and shows that the required changes are not that big.
On 2017/03/18 00:00:21, servolk wrote: > On 2017/03/17 23:42:36, xhwang_slow wrote: > > On 2017/03/17 22:43:24, DaleCurtis wrote: > > > Sorry don't have time to look at this until next week. > > > > Shall we have a meeting talking about the design first before we get into > > implementation review? :) > > Sure, I can schedule something next week. But if you get a chance, please take a > look at this CL as well, it's a prototype, but it demonstrates the option 1 > described in the go/media-track-switching doc and shows that the required > changes are not that big. Sure, I'll take a look as a preparation for the meeting.
The CQ bit was checked by servolk@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by servolk@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2017/03/18 00:05:39, xhwang_slow wrote: > On 2017/03/18 00:00:21, servolk wrote: > > On 2017/03/17 23:42:36, xhwang_slow wrote: > > > On 2017/03/17 22:43:24, DaleCurtis wrote: > > > > Sorry don't have time to look at this until next week. > > > > > > Shall we have a meeting talking about the design first before we get into > > > implementation review? :) > > > > Sure, I can schedule something next week. But if you get a chance, please take > a > > look at this CL as well, it's a prototype, but it demonstrates the option 1 > > described in the go/media-track-switching doc and shows that the required > > changes are not that big. > > Sure, I'll take a look as a preparation for the meeting. It looks like I've actually ran into some unanticipated issue here. Now that we are planning on landing this, I've looked more carefully at trybot failures and android_n5x_swarming_rel trybot fails are not just flakes. I'm not sure how to run those tests locally, but I did some investigation by extracting some code from this CL and running it through trybots and I narrowed it down as much as I could. I believe that there failures are somehow caused by the fact that before this CL we used to create audio and video decoders on the main thread, and with this CL that's been moved to the media thread. Further I'm going to be talking about audio decoders for brevity, but I believe the situation is exactly the same for video decoders. So currently the audio decoders are created by calling WMPI::CreateRenderer -> DefaultRendererFactory::CreateRenderer -> DefaultRendererFactory::CreateAudioDecoders. This whole call chain happens on the main thread. But in this CL I've made the CreateAudioDecoders to be deferred and now it's going to be called be AudioRendererImpl::Initialize, which happens to run on the media thread. And that seems to be working fine on all platforms, except Android. Xiaohan, do you know what's so special about Android platform in this case? Is there some kind of Android OS limitation that makes it necessary to create audio/video decoders on the main thread? I've tried to find all CreateAudioDecoders implementation in Chrome and so far I can see only two different implementations (https://cs.chromium.org/search/?q=createaudiodecoders+package:%5Echromium$&ty...). Obviously the DefaultRendererFactory::CreateAudioDecoders doesn't care which thread it is called on, since all other trybots are green. Is MojoDecoderFactory::CreateAudioDecoders only used by Android? The code in MojoDecoderFactory::CreateAudioDecoders is pretty simple I don't see any obvious reasons why it wouldn't work on the media thread. Xiaohan, any ideas? Does mojo require the service_manager::GetInterface to be called on the main thread?
avayvod@chromium.org changed reviewers: + avayvod@chromium.org
Note there's a pattern of overriding the default renderer factory for other reasons: - on desktop it's used by media remoting (ping miu@) - on Android it'll be used for HLS streams (ping tguilbert@) These might not care about the thread in general but I assume that both assume the main thread atm :)
(sorry to reply this message I have to publish some comments I already have on this CL; I am still looking) whywhat: Thanks for pointing out the existence of other Renderers. I think the scope of this CL is strictly within RendererImpl, so other Renderers should not be affected. servolk: It's been a long-time assumption that many media components (e.g. decoders) are created on the main thread, then used on the media thread. The original reason is that a lot of stuff needed for decoder creation are hosted by RenderFrameImpl, which can (mostly) only be accessed on the main thread. For mojo, typically it can only be used on one thread, where the mojo binding happens, so I won't be surprised if that's causing the problem. Given the async nature of mojo connections, I think it should be easy to forward the media::mojom::AudioDecoderRequest from the media thread to the main thread. If that indeed is the problem, you can probably fix MediaInterfaceProvider to make it safe to call from any thread (currently it's single threaded): basically forward the GetInterface() call to the main thread if it's called on a non-main thread :) https://codereview.chromium.org/2684103005/diff/160001/media/renderers/audio_... File media/renderers/audio_renderer_impl.h (right): https://codereview.chromium.org/2684103005/diff/160001/media/renderers/audio_... media/renderers/audio_renderer_impl.h:101: // when it is in a kFlushed and stopped (StopTicking) state. Please update AudioRenderer interface documentation about when Initialize() can be called. https://codereview.chromium.org/2684103005/diff/160001/media/renderers/defaul... File media/renderers/default_renderer_factory.cc (right): https://codereview.chromium.org/2684103005/diff/160001/media/renderers/defaul... media/renderers/default_renderer_factory.cc:108: // owns RendererImpl and thus AudioRendererImpl. Checked WMPI, |renderer_factory_| will be destroyed before the |pipeline_controller_| during destruction, so this comment isn't sufficient. You can probably say RendererImpl and thus AudioRendererImpl will be destroyed explicit in WMPI's dtor (pipeline_controller_.Stop()), which happens before |renderer_factory_| is destructed. https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... File media/renderers/renderer_impl.h (right): https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... media/renderers/renderer_impl.h:109: void RestartAudioRenderer(DemuxerStream* stream, base::TimeDelta time); Please add a comment about what the |stream| is, and what the |time| is for. https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... media/renderers/renderer_impl.h:112: PipelineStatus status); naming nit: Are "Restart" and "Reinit" the same thing? Let's pick one an be consistent.
Meanwhile, please help check other decoder creation and make sure they are okay to be created on the media thread.
On 2017/03/22 21:53:21, whywhat wrote: > Note there's a pattern of overriding the default renderer factory for other > reasons: > - on desktop it's used by media remoting (ping miu@) > - on Android it'll be used for HLS streams (ping tguilbert@) > > These might not care about the thread in general but I assume that both assume > the main thread atm :) Thanks, it's good to know this and I'll keep it in mind, although as Xiaohan mentioned this CL currently only affects the RendererImpl. As he also explained below, typically the decoders are created on the main thread, but are used on the media thread, which is what got me confused.
On 2017/03/22 22:31:13, xhwang_slow wrote: > (sorry to reply this message I have to publish some comments I already have on > this CL; I am still looking) > > whywhat: Thanks for pointing out the existence of other Renderers. I think the > scope of this CL is strictly within RendererImpl, so other Renderers should not > be affected. > > servolk: It's been a long-time assumption that many media components (e.g. > decoders) are created on the main thread, then used on the media thread. The > original reason is that a lot of stuff needed for decoder creation are hosted by > RenderFrameImpl, which can (mostly) only be accessed on the main thread. > > For mojo, typically it can only be used on one thread, where the mojo binding > happens, so I won't be surprised if that's causing the problem. Given the async > nature of mojo connections, I think it should be easy to forward the > media::mojom::AudioDecoderRequest from the media thread to the main thread. If > that indeed is the problem, you can probably fix MediaInterfaceProvider to make > it safe to call from any thread (currently it's single threaded): basically > forward the GetInterface() call to the main thread if it's called on a non-main > thread :) > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/audio_... > File media/renderers/audio_renderer_impl.h (right): > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/audio_... > media/renderers/audio_renderer_impl.h:101: // when it is in a kFlushed and > stopped (StopTicking) state. > Please update AudioRenderer interface documentation about when Initialize() can > be called. > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/defaul... > File media/renderers/default_renderer_factory.cc (right): > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/defaul... > media/renderers/default_renderer_factory.cc:108: // owns RendererImpl and thus > AudioRendererImpl. > Checked WMPI, |renderer_factory_| will be destroyed before the > |pipeline_controller_| during destruction, so this comment isn't sufficient. You > can probably say RendererImpl and thus AudioRendererImpl will be destroyed > explicit in WMPI's dtor (pipeline_controller_.Stop()), which happens before > |renderer_factory_| is destructed. > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... > File media/renderers/renderer_impl.h (right): > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... > media/renderers/renderer_impl.h:109: void RestartAudioRenderer(DemuxerStream* > stream, base::TimeDelta time); > Please add a comment about what the |stream| is, and what the |time| is for. > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... > media/renderers/renderer_impl.h:112: PipelineStatus status); > naming nit: Are "Restart" and "Reinit" the same thing? Let's pick one an be > consistent. Thanks, Xiaohan, that's very useful context. Just to make sure that I understand your suggestion correctly, what you are proposing to do is something like this: https://codereview.chromium.org/2771553003 ? I'll give it a try shortly.
On 2017/03/22 23:32:51, servolk wrote: > On 2017/03/22 22:31:13, xhwang_slow wrote: > > (sorry to reply this message I have to publish some comments I already have on > > this CL; I am still looking) > > > > whywhat: Thanks for pointing out the existence of other Renderers. I think the > > scope of this CL is strictly within RendererImpl, so other Renderers should > not > > be affected. > > > > servolk: It's been a long-time assumption that many media components (e.g. > > decoders) are created on the main thread, then used on the media thread. The > > original reason is that a lot of stuff needed for decoder creation are hosted > by > > RenderFrameImpl, which can (mostly) only be accessed on the main thread. > > > > For mojo, typically it can only be used on one thread, where the mojo binding > > happens, so I won't be surprised if that's causing the problem. Given the > async > > nature of mojo connections, I think it should be easy to forward the > > media::mojom::AudioDecoderRequest from the media thread to the main thread. If > > that indeed is the problem, you can probably fix MediaInterfaceProvider to > make > > it safe to call from any thread (currently it's single threaded): basically > > forward the GetInterface() call to the main thread if it's called on a > non-main > > thread :) > > > > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/audio_... > > File media/renderers/audio_renderer_impl.h (right): > > > > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/audio_... > > media/renderers/audio_renderer_impl.h:101: // when it is in a kFlushed and > > stopped (StopTicking) state. > > Please update AudioRenderer interface documentation about when Initialize() > can > > be called. > > > > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/defaul... > > File media/renderers/default_renderer_factory.cc (right): > > > > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/defaul... > > media/renderers/default_renderer_factory.cc:108: // owns RendererImpl and thus > > AudioRendererImpl. > > Checked WMPI, |renderer_factory_| will be destroyed before the > > |pipeline_controller_| during destruction, so this comment isn't sufficient. > You > > can probably say RendererImpl and thus AudioRendererImpl will be destroyed > > explicit in WMPI's dtor (pipeline_controller_.Stop()), which happens before > > |renderer_factory_| is destructed. > > > > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... > > File media/renderers/renderer_impl.h (right): > > > > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... > > media/renderers/renderer_impl.h:109: void RestartAudioRenderer(DemuxerStream* > > stream, base::TimeDelta time); > > Please add a comment about what the |stream| is, and what the |time| is for. > > > > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... > > media/renderers/renderer_impl.h:112: PipelineStatus status); > > naming nit: Are "Restart" and "Reinit" the same thing? Let's pick one an be > > consistent. > > Thanks, Xiaohan, that's very useful context. Just to make sure that I understand > your suggestion correctly, what you are proposing to do is something like this: > https://codereview.chromium.org/2771553003 ? > I'll give it a try shortly. Thanks for the quick CL. That is exactly what I suggested.
The CQ bit was checked by servolk@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.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2684103005/diff/160001/media/renderers/audio_... File media/renderers/audio_renderer_impl.h (right): https://codereview.chromium.org/2684103005/diff/160001/media/renderers/audio_... media/renderers/audio_renderer_impl.h:101: // when it is in a kFlushed and stopped (StopTicking) state. On 2017/03/22 22:31:12, xhwang_slow wrote: > Please update AudioRenderer interface documentation about when Initialize() can > be called. Done. https://codereview.chromium.org/2684103005/diff/160001/media/renderers/defaul... File media/renderers/default_renderer_factory.cc (right): https://codereview.chromium.org/2684103005/diff/160001/media/renderers/defaul... media/renderers/default_renderer_factory.cc:108: // owns RendererImpl and thus AudioRendererImpl. On 2017/03/22 22:31:12, xhwang_slow wrote: > Checked WMPI, |renderer_factory_| will be destroyed before the > |pipeline_controller_| during destruction, so this comment isn't sufficient. You > can probably say RendererImpl and thus AudioRendererImpl will be destroyed > explicit in WMPI's dtor (pipeline_controller_.Stop()), which happens before > |renderer_factory_| is destructed. Done. https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... File media/renderers/renderer_impl.h (right): https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... media/renderers/renderer_impl.h:109: void RestartAudioRenderer(DemuxerStream* stream, base::TimeDelta time); On 2017/03/22 22:31:12, xhwang_slow wrote: > Please add a comment about what the |stream| is, and what the |time| is for. Done. https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... media/renderers/renderer_impl.h:112: PipelineStatus status); On 2017/03/22 22:31:12, xhwang_slow wrote: > naming nit: Are "Restart" and "Reinit" the same thing? Let's pick one an be > consistent. No restart and reinit are different things. Reinit completely reinitializes the renderer with a different DemuxerStream, which includes re-creating audio decoders and re-running the decoder selection. Whereas restart simply restarts the playback in audio renderer, for example after the media track has been disabled and became re-enabled. I've made the comments above more detailed to explain this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/23 17:08:10, servolk wrote: > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/audio_... > File media/renderers/audio_renderer_impl.h (right): > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/audio_... > media/renderers/audio_renderer_impl.h:101: // when it is in a kFlushed and > stopped (StopTicking) state. > On 2017/03/22 22:31:12, xhwang_slow wrote: > > Please update AudioRenderer interface documentation about when Initialize() > can > > be called. > > Done. > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/defaul... > File media/renderers/default_renderer_factory.cc (right): > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/defaul... > media/renderers/default_renderer_factory.cc:108: // owns RendererImpl and thus > AudioRendererImpl. > On 2017/03/22 22:31:12, xhwang_slow wrote: > > Checked WMPI, |renderer_factory_| will be destroyed before the > > |pipeline_controller_| during destruction, so this comment isn't sufficient. > You > > can probably say RendererImpl and thus AudioRendererImpl will be destroyed > > explicit in WMPI's dtor (pipeline_controller_.Stop()), which happens before > > |renderer_factory_| is destructed. > > Done. > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... > File media/renderers/renderer_impl.h (right): > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... > media/renderers/renderer_impl.h:109: void RestartAudioRenderer(DemuxerStream* > stream, base::TimeDelta time); > On 2017/03/22 22:31:12, xhwang_slow wrote: > > Please add a comment about what the |stream| is, and what the |time| is for. > > Done. > > https://codereview.chromium.org/2684103005/diff/160001/media/renderers/render... > media/renderers/renderer_impl.h:112: PipelineStatus status); > On 2017/03/22 22:31:12, xhwang_slow wrote: > > naming nit: Are "Restart" and "Reinit" the same thing? Let's pick one an be > > consistent. > > No restart and reinit are different things. Reinit completely reinitializes the > renderer with a different DemuxerStream, which includes re-creating audio > decoders and re-running the decoder selection. Whereas restart simply restarts > the playback in audio renderer, for example after the media track has been > disabled and became re-enabled. I've made the comments above more detailed to > explain this. PTAL. I've fixed all the comments so far and with the change to allow mojo audio/video decoder creation from the media thread all the unit tests pass now.
https://codereview.chromium.org/2684103005/diff/200001/content/renderer/media... File content/renderer/media/media_interface_provider.cc (right): https://codereview.chromium.org/2684103005/diff/200001/content/renderer/media... content/renderer/media/media_interface_provider.cc:33: weak_factory_.GetWeakPtr(), interface_name, Can't vend WeakPtrs from a different thread like this; it's not thread safe. You at least need to bind and keep alive one WeakPtr on |task_runner_|. https://codereview.chromium.org/2684103005/diff/200001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/200001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:339: if (state_ == kFlushed) Sink should already be stopped by this point? https://codereview.chromium.org/2684103005/diff/200001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/200001/media/renderers/render... media/renderers/renderer_impl.cc:288: DCHECK_EQ(stream, current_video_stream_); Seems like this could change if called in fast succession? I don't see a callback indicating the stream switch has completed before allowing further calls.
This will need layout tests or browser tests for end to end integration testing.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2684103005/diff/200001/content/renderer/media... File content/renderer/media/media_interface_provider.cc (right): https://codereview.chromium.org/2684103005/diff/200001/content/renderer/media... content/renderer/media/media_interface_provider.cc:33: weak_factory_.GetWeakPtr(), interface_name, On 2017/03/24 19:35:22, DaleCurtis wrote: > Can't vend WeakPtrs from a different thread like this; it's not thread safe. You > at least need to bind and keep alive one WeakPtr on |task_runner_|. Done (although I'm a little surprised. shouldn't there be a dcheck about this if it's unsafe? i.e. shouldn't we have a dcheck(same thread as weak factory ctor) inside the GetWeakPtr to prevent unsafe usage?). https://codereview.chromium.org/2684103005/diff/200001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/200001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:339: if (state_ == kFlushed) On 2017/03/24 19:35:22, DaleCurtis wrote: > Sink should already be stopped by this point? No, as far as I can see AudioRendererImpl stops the sink only in the destructor. AudioRendererImpl::StopRendering_Locked just pauses the sink. Since calling Initialize from kFlushed state only happens when switching audio tracks, I figured it's probably safest to stop the sink in case the new track has a different sampling rate or number of channels. https://codereview.chromium.org/2684103005/diff/200001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/200001/media/renderers/render... media/renderers/renderer_impl.cc:288: DCHECK_EQ(stream, current_video_stream_); On 2017/03/24 19:35:22, DaleCurtis wrote: > Seems like this could change if called in fast succession? I don't see a > callback indicating the stream switch has completed before allowing further > calls. No, it should be fine. Here is how it works: when the status of any track/stream changes, we set a flag (either restarting_audio_ or restarting_video_ in RendererImpl::OnStreamStatusChanged, see https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=68e...). As long as one of those flags is set we are going to postpone handling of other track status changes, including status changes of the same track, see https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=68e.... The flags are reset in RendererImpl::OnStreamRestartCompleted, which only gets invoked after we have finished processing the current status change. This mechanism ensures that chains of async calls involved in handling track status changes (OnStreamStatusChanged -> RestartAudio/VideoRenderer -> (optionally, in case of track switch) OnAudio/VideoRendererReinitCompleted -> OnBufferingStateChange -> OnStreamRestartCompleted) are serialized and never overlap.
servolk: Sorry I have an important CL to finish today. I'll take a look at your CL over the weekend.
On 2017/03/24 19:35:48, DaleCurtis wrote: > This will need layout tests or browser tests for end to end integration testing. Sure, I'll look into adding those shortly, only I'm not sure how much verification (other than the fact that the browser didn't crash) we can do in there. The new pipeline integration tests in this CL are already pretty close to what the blink level will do in those tests - just start playback and do some track switching using track ids.
https://codereview.chromium.org/2684103005/diff/200001/content/renderer/media... File content/renderer/media/media_interface_provider.cc (right): https://codereview.chromium.org/2684103005/diff/200001/content/renderer/media... content/renderer/media/media_interface_provider.cc:33: weak_factory_.GetWeakPtr(), interface_name, On 2017/03/24 at 21:15:32, servolk wrote: > On 2017/03/24 19:35:22, DaleCurtis wrote: > > Can't vend WeakPtrs from a different thread like this; it's not thread safe. You > > at least need to bind and keep alive one WeakPtr on |task_runner_|. > > Done (although I'm a little surprised. shouldn't there be a dcheck about this if it's unsafe? i.e. shouldn't we have a dcheck(same thread as weak factory ctor) inside the GetWeakPtr to prevent unsafe usage?). Hard to DCHECK since it's a raciness issue. https://codereview.chromium.org/2684103005/diff/200001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/200001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:339: if (state_ == kFlushed) On 2017/03/24 at 21:15:32, servolk wrote: > On 2017/03/24 19:35:22, DaleCurtis wrote: > > Sink should already be stopped by this point? > > No, as far as I can see AudioRendererImpl stops the sink only in the destructor. AudioRendererImpl::StopRendering_Locked just pauses the sink. Since calling Initialize from kFlushed state only happens when switching audio tracks, I figured it's probably safest to stop the sink in case the new track has a different sampling rate or number of channels. Sorry, I meant that the sink shouldn't be issuing callbacks at this point? Do we need to Stop() the sink here or is a pause/play cycle enough? Otherwise I think this will result in a AudioOutputDevice being dropped and recreated (w/ shared memory and other overhead attached).
https://codereview.chromium.org/2684103005/diff/200001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/200001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:339: if (state_ == kFlushed) On 2017/03/24 21:30:38, DaleCurtis wrote: > On 2017/03/24 at 21:15:32, servolk wrote: > > On 2017/03/24 19:35:22, DaleCurtis wrote: > > > Sink should already be stopped by this point? > > > > No, as far as I can see AudioRendererImpl stops the sink only in the > destructor. AudioRendererImpl::StopRendering_Locked just pauses the sink. Since > calling Initialize from kFlushed state only happens when switching audio tracks, > I figured it's probably safest to stop the sink in case the new track has a > different sampling rate or number of channels. > > Sorry, I meant that the sink shouldn't be issuing callbacks at this point? Do we > need to Stop() the sink here or is a pause/play cycle enough? Otherwise I think > this will result in a AudioOutputDevice being dropped and recreated (w/ shared > memory and other overhead attached). > I think that currently a simple play/pause cycle may be not enough, because if I remove this or replace sink_->Stop() with sink_->Pause(), I'm getting a DCHECK when this method goes on and reinitializes the audio buffer stream: [1:13:0324/151216.804515:FATAL:webaudiosourceprovider_impl.cc(188)] Check failed: state_ == kStopped (1 vs. 0) #0 0x7f14f6494ccb base::debug::StackTrace::StackTrace() #1 0x7f14f649335c base::debug::StackTrace::StackTrace() #2 0x7f14f650124f logging::LogMessage::~LogMessage() #3 0x7f14e11f24c7 media::WebAudioSourceProviderImpl::Initialize() #4 0x7f14ecceb2d9 media::AudioRendererImpl::OnAudioBufferStreamInitialized()
https://codereview.chromium.org/2684103005/diff/200001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/200001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:339: if (state_ == kFlushed) On 2017/03/24 at 22:18:13, servolk wrote: > On 2017/03/24 21:30:38, DaleCurtis wrote: > > On 2017/03/24 at 21:15:32, servolk wrote: > > > On 2017/03/24 19:35:22, DaleCurtis wrote: > > > > Sink should already be stopped by this point? > > > > > > No, as far as I can see AudioRendererImpl stops the sink only in the > > destructor. AudioRendererImpl::StopRendering_Locked just pauses the sink. Since > > calling Initialize from kFlushed state only happens when switching audio tracks, > > I figured it's probably safest to stop the sink in case the new track has a > > different sampling rate or number of channels. > > > > Sorry, I meant that the sink shouldn't be issuing callbacks at this point? Do we > > need to Stop() the sink here or is a pause/play cycle enough? Otherwise I think > > this will result in a AudioOutputDevice being dropped and recreated (w/ shared > > memory and other overhead attached). > > > > I think that currently a simple play/pause cycle may be not enough, because if I remove this or replace sink_->Stop() with sink_->Pause(), I'm getting a DCHECK when this method goes on and reinitializes the audio buffer stream: > [1:13:0324/151216.804515:FATAL:webaudiosourceprovider_impl.cc(188)] Check failed: state_ == kStopped (1 vs. 0) > #0 0x7f14f6494ccb base::debug::StackTrace::StackTrace() > #1 0x7f14f649335c base::debug::StackTrace::StackTrace() > #2 0x7f14f650124f logging::LogMessage::~LogMessage() > #3 0x7f14e11f24c7 media::WebAudioSourceProviderImpl::Initialize() > #4 0x7f14ecceb2d9 media::AudioRendererImpl::OnAudioBufferStreamInitialized() Yes, I was thinking the code would be smart enough to avoid the re-initialize when possible. Though I just remembered you configuration may change, so perhaps it's unavoidable.
The CQ bit was checked by servolk@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...
On 2017/03/24 21:18:05, servolk wrote: > On 2017/03/24 19:35:48, DaleCurtis wrote: > > This will need layout tests or browser tests for end to end integration > testing. > > Sure, I'll look into adding those shortly, only I'm not sure how much > verification (other than the fact that the browser didn't crash) we can do in > there. The new pipeline integration tests in this CL are already pretty close to > what the blink level will do in those tests - just start playback and do some > track switching using track ids. Ok, I've added a new layout test for media track switching in the latest patchset. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I haven't looked at the tests yet. But the implementation makes sense to me! I have a few comments, mostly nits. https://codereview.chromium.org/2684103005/diff/260001/content/renderer/media... File content/renderer/media/media_interface_provider.cc (right): https://codereview.chromium.org/2684103005/diff/260001/content/renderer/media... content/renderer/media/media_interface_provider.cc:36: } nit: empty line https://codereview.chromium.org/2684103005/diff/260001/content/renderer/media... File content/renderer/media/media_interface_provider.h (right): https://codereview.chromium.org/2684103005/diff/260001/content/renderer/media... content/renderer/media/media_interface_provider.h:25: // This class is single threaded. Please help fix this comment :) https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1043: bool first_audio_track = true; is_first_audio_track ? https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1051: /*enabled*/ first_audio_track); This line doesn't make sense now :) https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1058: /*selected*/ first_video_track); ditto https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:61: DCHECK(!create_audio_decoders_cb_.is_null()); FYI: I think you can do this now: DCHECK(create_audio_decoders_cb_); https://cs.chromium.org/chromium/src/base/callback_internal.h?rcl=5f5b9df5535... But feel free to ignore for consistency with existing code https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... File media/renderers/audio_renderer_impl.h (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... media/renderers/audio_renderer_impl.h:101: // when it is in a kFlushed and stopped (StopTicking) state. AudioRenderer API only says it can be reinitialized in kFlushed state. Why do we need "stopped state" here? https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... media/renderers/audio_renderer_impl.h:259: CreateAudioDecodersCB create_audio_decoders_cb_; FYI, we do have a DecoderFactory interface: https://cs.chromium.org/chromium/src/media/base/decoder_factory.h I wonder whether it's a good idea to consolidate that with this callback (in a later CL) https://codereview.chromium.org/2684103005/diff/260001/media/renderers/defaul... File media/renderers/default_renderer_factory.h (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/defaul... media/renderers/default_renderer_factory.h:27: using CreateVideoDecodersCB = base::Callback<ScopedVector<VideoDecoder>()>; nit: Maybe use RepeatingCallback to make it clear this can be used repeatedly? https://cs.chromium.org/chromium/src/docs/callback.md?rcl=2fc3ba8587526eb5e5f... https://codereview.chromium.org/2684103005/diff/260001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/render... media/renderers/renderer_impl.cc:260: base::TimeDelta time) { I see what you are doing, but feel it's a bit hard to read that RestartVideoRenderer() schedules OnVideoRendererReinitCompleted(), and OnVideoRendererReinitCompleted() in turn calls RestartVideoRenderer() again. So, in OnStreamStatusChanged(), if stream->type() == DemuxerStream::VIDEO, we know we will reinitialize the VideoRenderer. Is that correct? In that case, instead of calling RestartVideoRenderer(), which actually reinitializes the VideoRenderer and returns, can we call ReinitializeVideoRenderer(), which is basically the block of line 266-273? https://codereview.chromium.org/2684103005/diff/260001/media/renderers/render... media/renderers/renderer_impl.cc:275: } nit: empty line here https://codereview.chromium.org/2684103005/diff/260001/media/renderers/render... File media/renderers/renderer_impl.h (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/render... media/renderers/renderer_impl.h:126: void OnAudioRendererReinitCompleted(DemuxerStream* stream, nit: OnAudioRendererReinitialized, or OnAudioRendererReinitializeDone to be consistent with line 99 https://codereview.chromium.org/2684103005/diff/260001/media/renderers/video_... File media/renderers/video_renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/video_... media/renderers/video_renderer_impl.cc:42: video_frame_stream_(nullptr), not needed https://codereview.chromium.org/2684103005/diff/260001/media/renderers/video_... media/renderers/video_renderer_impl.cc:160: task_runner_, worker_task_runner_, gpu_factories_)); OOC, why do we need to create a new GpuMemoryBufferVideoFramePool upon reinit? If there's a legit reason, add a comment?
Overall lg, but can you clarify the expectations of what is allowed to happen between the time a new track is selected and reinitialization completes? What happens with calls like playbackRate, seek(), etc?
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2684103005/diff/260001/content/renderer/media... File content/renderer/media/media_interface_provider.cc (right): https://codereview.chromium.org/2684103005/diff/260001/content/renderer/media... content/renderer/media/media_interface_provider.cc:36: } On 2017/03/27 19:13:53, xhwang_slow wrote: > nit: empty line Done. https://codereview.chromium.org/2684103005/diff/260001/content/renderer/media... File content/renderer/media/media_interface_provider.h (right): https://codereview.chromium.org/2684103005/diff/260001/content/renderer/media... content/renderer/media/media_interface_provider.h:25: // This class is single threaded. On 2017/03/27 19:13:53, xhwang_slow wrote: > Please help fix this comment :) Done. https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1043: bool first_audio_track = true; On 2017/03/27 19:13:53, xhwang_slow wrote: > is_first_audio_track ? Done. https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1051: /*enabled*/ first_audio_track); On 2017/03/27 19:13:53, xhwang_slow wrote: > This line doesn't make sense now :) If you are talking about the comment, it still does make sense. The comment clarifies that the last parameter is 'bool enabled' and tells the client whether the added media track should be in the enabled or disabled state initially. It just so happens that we want only the first track to be enabled initially, and so we pass the is_first_audio_track in there. Would it be clearer to change comment to something like /* enabled= */ ? https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1058: /*selected*/ first_video_track); On 2017/03/27 19:13:53, xhwang_slow wrote: > ditto Acknowledged. https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:61: DCHECK(!create_audio_decoders_cb_.is_null()); On 2017/03/27 19:13:53, xhwang_slow wrote: > FYI: > > I think you can do this now: > > DCHECK(create_audio_decoders_cb_); > > https://cs.chromium.org/chromium/src/base/callback_internal.h?rcl=5f5b9df5535... > > But feel free to ignore for consistency with existing code Nice, thanks, I didn't know about this. Done. https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... File media/renderers/audio_renderer_impl.h (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... media/renderers/audio_renderer_impl.h:101: // when it is in a kFlushed and stopped (StopTicking) state. On 2017/03/27 19:13:53, xhwang_slow wrote: > AudioRenderer API only says it can be reinitialized in kFlushed state. Why do we > need "stopped state" here? AFAIR this must be a left over from some earlier attempts to make it work. Now that we are going to stop the sink automatically in AudioRendererImpl::Initialize when it's called from kFlushed state, this should no longer be necessary, so I've removed the stopped state from the comment. https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... media/renderers/audio_renderer_impl.h:259: CreateAudioDecodersCB create_audio_decoders_cb_; On 2017/03/27 19:13:53, xhwang_slow wrote: > FYI, we do have a DecoderFactory interface: > > https://cs.chromium.org/chromium/src/media/base/decoder_factory.h > > I wonder whether it's a good idea to consolidate that with this callback (in a > later CL) Yes, I saw that, but for now the DefaultRendererFactory doesn't implement the DecoderFactory interface, so it was easier to get a callback rather than a full DecoderFactory. Besides, here we only need to create audio decoders (and in video renderer we need to create only video decoders), but in the DecoderFactory interface these two methods are tied together. I agree that we should probably revisit this design later. https://codereview.chromium.org/2684103005/diff/260001/media/renderers/defaul... File media/renderers/default_renderer_factory.h (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/defaul... media/renderers/default_renderer_factory.h:27: using CreateVideoDecodersCB = base::Callback<ScopedVector<VideoDecoder>()>; On 2017/03/27 19:13:53, xhwang_slow wrote: > nit: Maybe use RepeatingCallback to make it clear this can be used repeatedly? > > https://cs.chromium.org/chromium/src/docs/callback.md?rcl=2fc3ba8587526eb5e5f... Done. https://codereview.chromium.org/2684103005/diff/260001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/render... media/renderers/renderer_impl.cc:260: base::TimeDelta time) { On 2017/03/27 19:13:53, xhwang_slow wrote: > I see what you are doing, but feel it's a bit hard to read that > RestartVideoRenderer() schedules OnVideoRendererReinitCompleted(), and > OnVideoRendererReinitCompleted() in turn calls RestartVideoRenderer() again. > > So, in OnStreamStatusChanged(), if stream->type() == DemuxerStream::VIDEO, we > know we will reinitialize the VideoRenderer. Is that correct? In that case, > instead of calling RestartVideoRenderer(), which actually reinitializes the > VideoRenderer and returns, can we call ReinitializeVideoRenderer(), which is > basically the block of line 266-273? Ok, sure, I've refactored it a bit to hopefully make it more readable. https://codereview.chromium.org/2684103005/diff/260001/media/renderers/render... media/renderers/renderer_impl.cc:275: } On 2017/03/27 19:13:53, xhwang_slow wrote: > nit: empty line here Done. https://codereview.chromium.org/2684103005/diff/260001/media/renderers/render... File media/renderers/renderer_impl.h (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/render... media/renderers/renderer_impl.h:126: void OnAudioRendererReinitCompleted(DemuxerStream* stream, On 2017/03/27 19:13:53, xhwang_slow wrote: > nit: OnAudioRendererReinitialized, or OnAudioRendererReinitializeDone to be > consistent with line 99 Done. https://codereview.chromium.org/2684103005/diff/260001/media/renderers/video_... File media/renderers/video_renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/video_... media/renderers/video_renderer_impl.cc:42: video_frame_stream_(nullptr), On 2017/03/27 19:13:53, xhwang_slow wrote: > not needed Done. https://codereview.chromium.org/2684103005/diff/260001/media/renderers/video_... media/renderers/video_renderer_impl.cc:160: task_runner_, worker_task_runner_, gpu_factories_)); On 2017/03/27 19:13:53, xhwang_slow wrote: > OOC, why do we need to create a new GpuMemoryBufferVideoFramePool upon reinit? > If there's a legit reason, add a comment? Well, if we are switching from let's say 8-bit H264 video track to a 10bit H264 track, then the GPU buffers would probably need to be reallocated. I've added a comment about it. Also, I think we can save a some memory by adding an else clause an releasing |gpu_memory_buffer_pool_| if we know GPU is not going to be used.
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: + dcastagna@chromium.org
+dcastagna: see my question below Here's my last round of comments. I think Dale made a good point that we need to watch out for Renderer calls when we are pending reinitialization. Can you help check that? Maybe adding more RendererImplTest to cover these cases, and the new code path? https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1051: /*enabled*/ first_audio_track); On 2017/03/27 22:34:12, servolk wrote: > On 2017/03/27 19:13:53, xhwang_slow wrote: > > This line doesn't make sense now :) > > If you are talking about the comment, it still does make sense. The comment > clarifies that the last parameter is 'bool enabled' and tells the client whether > the added media track should be in the enabled or disabled state initially. It > just so happens that we want only the first track to be enabled initially, and > so we pass the is_first_audio_track in there. Would it be clearer to change > comment to something like /* enabled= */ ? I see. Thanks for the context. I think this style where you put comments before the variable is very rare and IMHO confusing. Also, probably add comments about why we only enable/select the first track? https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... File media/renderers/audio_renderer_impl.h (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... media/renderers/audio_renderer_impl.h:259: CreateAudioDecodersCB create_audio_decoders_cb_; On 2017/03/27 22:34:13, servolk wrote: > On 2017/03/27 19:13:53, xhwang_slow wrote: > > FYI, we do have a DecoderFactory interface: > > > > https://cs.chromium.org/chromium/src/media/base/decoder_factory.h > > > > I wonder whether it's a good idea to consolidate that with this callback (in a > > later CL) > > Yes, I saw that, but for now the DefaultRendererFactory doesn't implement the > DecoderFactory interface, so it was easier to get a callback rather than a full > DecoderFactory. Besides, here we only need to create audio decoders (and in > video renderer we need to create only video decoders), but in the DecoderFactory > interface these two methods are tied together. I agree that we should probably > revisit this design later. Could you please add a TODO? https://codereview.chromium.org/2684103005/diff/280001/content/renderer/media... File content/renderer/media/media_interface_provider.h (right): https://codereview.chromium.org/2684103005/diff/280001/content/renderer/media... content/renderer/media/media_interface_provider.h:15: #include "url/gurl.h" add include for WeakPtr* https://codereview.chromium.org/2684103005/diff/280001/content/renderer/media... content/renderer/media/media_interface_provider.h:26: // thread where constructor was called) if necessary. That's impl detail. You can probably just say "GetInterface() can be called on any thread". https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.cc:267: DVLOG(3) << __func__ << " stream=" << stream << " time=" << time.InSecondsF(); nit: here and everywhere else, media switching isn't very common, and worth a DVLOG(2) IMHO https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.cc:280: void RendererImpl::OnVideoRendererReinitialized(DemuxerStream* stream, nit: Why do we need to pass in the |stream| here and in RestartVideoRenderer(), given we already set \current_video_stream_\? https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... File media/renderers/renderer_impl.h (right): https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.h:66: base::TimeDelta time); This doesn't need to be public. Also it would be nice to be grouped together with other stream-switching functions, e.g. Reinitialize*Renderer(). https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.h:114: // where the switch occured. nit: At this level, we are dealing with DemuxerStream swiching (StatusChange) and we don't need to mention "media track". People looking at this code would have no idea what a media track is. https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.h:125: // media track has been disabled and re-enabled. The |stream| parameter ditto about "media track" https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.h:146: BufferingState new_buffering_state); empty line https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.h:155: BufferingState new_buffering_state); empty line https://codereview.chromium.org/2684103005/diff/280001/media/renderers/video_... File media/renderers/video_renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/280001/media/renderers/video_... media/renderers/video_renderer_impl.cc:162: gpu_memory_buffer_pool_.reset(new GpuMemoryBufferVideoFramePool( What if we have outstanding VideoFrames out there? Will this cause black frames? I think NOT because each frame will keep a ref-count to the PoolImpl: https://cs.chromium.org/chromium/src/media/video/gpu_memory_buffer_video_fram... But I am not sure :) Maybe we should have a GpuMemoryBufferVideoFramePool::Reset() method such that it'll hide these implementation details from it's clients. +dcastagna to double check
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1051: /*enabled*/ first_audio_track); On 2017/03/29 00:16:47, xhwang_slow wrote: > On 2017/03/27 22:34:12, servolk wrote: > > On 2017/03/27 19:13:53, xhwang_slow wrote: > > > This line doesn't make sense now :) > > > > If you are talking about the comment, it still does make sense. The comment > > clarifies that the last parameter is 'bool enabled' and tells the client > whether > > the added media track should be in the enabled or disabled state initially. It > > just so happens that we want only the first track to be enabled initially, and > > so we pass the is_first_audio_track in there. Would it be clearer to change > > comment to something like /* enabled= */ ? > > I see. Thanks for the context. I think this style where you put comments before > the variable is very rare and IMHO confusing. > > Also, probably add comments about why we only enable/select the first track? Ok, I guess I can completely remove the parameter comments, if that's confusing. Done. https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... File media/renderers/audio_renderer_impl.h (right): https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... media/renderers/audio_renderer_impl.h:259: CreateAudioDecodersCB create_audio_decoders_cb_; On 2017/03/29 00:16:47, xhwang_slow wrote: > On 2017/03/27 22:34:13, servolk wrote: > > On 2017/03/27 19:13:53, xhwang_slow wrote: > > > FYI, we do have a DecoderFactory interface: > > > > > > https://cs.chromium.org/chromium/src/media/base/decoder_factory.h > > > > > > I wonder whether it's a good idea to consolidate that with this callback (in > a > > > later CL) > > > > Yes, I saw that, but for now the DefaultRendererFactory doesn't implement the > > DecoderFactory interface, so it was easier to get a callback rather than a > full > > DecoderFactory. Besides, here we only need to create audio decoders (and in > > video renderer we need to create only video decoders), but in the > DecoderFactory > > interface these two methods are tied together. I agree that we should probably > > revisit this design later. > > Could you please add a TODO? Done. https://codereview.chromium.org/2684103005/diff/280001/content/renderer/media... File content/renderer/media/media_interface_provider.h (right): https://codereview.chromium.org/2684103005/diff/280001/content/renderer/media... content/renderer/media/media_interface_provider.h:15: #include "url/gurl.h" On 2017/03/29 00:16:47, xhwang_slow wrote: > add include for WeakPtr* Done. https://codereview.chromium.org/2684103005/diff/280001/content/renderer/media... content/renderer/media/media_interface_provider.h:26: // thread where constructor was called) if necessary. On 2017/03/29 00:16:47, xhwang_slow wrote: > That's impl detail. You can probably just say "GetInterface() can be called on > any thread". Done. https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.cc:267: DVLOG(3) << __func__ << " stream=" << stream << " time=" << time.InSecondsF(); On 2017/03/29 00:16:47, xhwang_slow wrote: > nit: here and everywhere else, media switching isn't very common, and worth a > DVLOG(2) IMHO Done. https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.cc:280: void RendererImpl::OnVideoRendererReinitialized(DemuxerStream* stream, On 2017/03/29 00:16:47, xhwang_slow wrote: > nit: Why do we need to pass in the |stream| here and in RestartVideoRenderer(), > given we already set \current_video_stream_\? Well, we could do without it, but it's just slightly nicer to have matching function signatures for Restart and Reinitialize, since it allows us to use a common parameter list at lines 242-245 and 258-261 above. https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... File media/renderers/renderer_impl.h (right): https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.h:66: base::TimeDelta time); On 2017/03/29 00:16:47, xhwang_slow wrote: > This doesn't need to be public. Also it would be nice to be grouped together > with other stream-switching functions, e.g. Reinitialize*Renderer(). Done. https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.h:114: // where the switch occured. On 2017/03/29 00:16:47, xhwang_slow wrote: > nit: At this level, we are dealing with DemuxerStream swiching (StatusChange) > and we don't need to mention "media track". People looking at this code would > have no idea what a media track is. Done. https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.h:125: // media track has been disabled and re-enabled. The |stream| parameter On 2017/03/29 00:16:47, xhwang_slow wrote: > ditto about "media track" Done. https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.h:146: BufferingState new_buffering_state); On 2017/03/29 00:16:47, xhwang_slow wrote: > empty line Done. https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... media/renderers/renderer_impl.h:155: BufferingState new_buffering_state); On 2017/03/29 00:16:47, xhwang_slow wrote: > empty line Done. https://codereview.chromium.org/2684103005/diff/280001/media/renderers/video_... File media/renderers/video_renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/280001/media/renderers/video_... media/renderers/video_renderer_impl.cc:162: gpu_memory_buffer_pool_.reset(new GpuMemoryBufferVideoFramePool( On 2017/03/29 00:16:47, xhwang_slow wrote: > What if we have outstanding VideoFrames out there? Will this cause black frames? > > I think NOT because each frame will keep a ref-count to the PoolImpl: > > https://cs.chromium.org/chromium/src/media/video/gpu_memory_buffer_video_fram... > > But I am not sure :) > > Maybe we should have a GpuMemoryBufferVideoFramePool::Reset() method such that > it'll hide these implementation details from it's clients. > > +dcastagna to double check Good question. I'm not familiar with GPU memory pool design, so I'll let dcastagna answer this. FWIW I didn't see notice any issues when testing video track switching on my macbook, but I'm not sure how to tell if GPU was used there or not.
On 2017/03/27 21:01:27, DaleCurtis wrote: > Overall lg, but can you clarify the expectations of what is allowed to happen > between the time a new track is selected and reinitialization completes? What > happens with calls like playbackRate, seek(), etc? Good question. I think playbackRate should be fine and completely unaffected by track switching (since playback rate is unchanged and known by the RendererImpl while the switch is happening). But seek() is a different matter. I think currently seek() might interfere with track switching (since both track switching and the seek operation involve flushing a/v renderers and we don't allow more than one pending seek operation at a time). I've spent a some time today trying to trigger an issue that would cause a dcheck or a crash when seek occurs while the media track switch is still being handled. But so far I haven't found a way to trigger. I will continue my attempts tomorrow. I believe that the right way to fix this is probably to postpone the seek operation if it happens when a media track switch is being processed, similar to how we postpone stream status notification handling if another status change in being handled (see https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=9dd...).
On 2017/03/29 at 00:16:48, xhwang wrote: > +dcastagna: see my question below > > Here's my last round of comments. I think Dale made a good point that we need to watch out for Renderer calls when we are pending reinitialization. Can you help check that? Maybe adding more RendererImplTest to cover these cases, and the new code path? > > https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... > media/blink/webmediaplayer_impl.cc:1051: /*enabled*/ first_audio_track); > On 2017/03/27 22:34:12, servolk wrote: > > On 2017/03/27 19:13:53, xhwang_slow wrote: > > > This line doesn't make sense now :) > > > > If you are talking about the comment, it still does make sense. The comment > > clarifies that the last parameter is 'bool enabled' and tells the client whether > > the added media track should be in the enabled or disabled state initially. It > > just so happens that we want only the first track to be enabled initially, and > > so we pass the is_first_audio_track in there. Would it be clearer to change > > comment to something like /* enabled= */ ? > > I see. Thanks for the context. I think this style where you put comments before the variable is very rare and IMHO confusing. > > Also, probably add comments about why we only enable/select the first track? > > https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... > File media/renderers/audio_renderer_impl.h (right): > > https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... > media/renderers/audio_renderer_impl.h:259: CreateAudioDecodersCB create_audio_decoders_cb_; > On 2017/03/27 22:34:13, servolk wrote: > > On 2017/03/27 19:13:53, xhwang_slow wrote: > > > FYI, we do have a DecoderFactory interface: > > > > > > https://cs.chromium.org/chromium/src/media/base/decoder_factory.h > > > > > > I wonder whether it's a good idea to consolidate that with this callback (in a > > > later CL) > > > > Yes, I saw that, but for now the DefaultRendererFactory doesn't implement the > > DecoderFactory interface, so it was easier to get a callback rather than a full > > DecoderFactory. Besides, here we only need to create audio decoders (and in > > video renderer we need to create only video decoders), but in the DecoderFactory > > interface these two methods are tied together. I agree that we should probably > > revisit this design later. > > Could you please add a TODO? > > https://codereview.chromium.org/2684103005/diff/280001/content/renderer/media... > File content/renderer/media/media_interface_provider.h (right): > > https://codereview.chromium.org/2684103005/diff/280001/content/renderer/media... > content/renderer/media/media_interface_provider.h:15: #include "url/gurl.h" > add include for WeakPtr* > > https://codereview.chromium.org/2684103005/diff/280001/content/renderer/media... > content/renderer/media/media_interface_provider.h:26: // thread where constructor was called) if necessary. > That's impl detail. You can probably just say "GetInterface() can be called on any thread". > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > File media/renderers/renderer_impl.cc (right): > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > media/renderers/renderer_impl.cc:267: DVLOG(3) << __func__ << " stream=" << stream << " time=" << time.InSecondsF(); > nit: here and everywhere else, media switching isn't very common, and worth a DVLOG(2) IMHO > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > media/renderers/renderer_impl.cc:280: void RendererImpl::OnVideoRendererReinitialized(DemuxerStream* stream, > nit: Why do we need to pass in the |stream| here and in RestartVideoRenderer(), given we already set \current_video_stream_\? > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > File media/renderers/renderer_impl.h (right): > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > media/renderers/renderer_impl.h:66: base::TimeDelta time); > This doesn't need to be public. Also it would be nice to be grouped together with other stream-switching functions, e.g. Reinitialize*Renderer(). > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > media/renderers/renderer_impl.h:114: // where the switch occured. > nit: At this level, we are dealing with DemuxerStream swiching (StatusChange) and we don't need to mention "media track". People looking at this code would have no idea what a media track is. > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > media/renderers/renderer_impl.h:125: // media track has been disabled and re-enabled. The |stream| parameter > ditto about "media track" > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > media/renderers/renderer_impl.h:146: BufferingState new_buffering_state); > empty line > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > media/renderers/renderer_impl.h:155: BufferingState new_buffering_state); > empty line > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/video_... > File media/renderers/video_renderer_impl.cc (right): > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/video_... > media/renderers/video_renderer_impl.cc:162: gpu_memory_buffer_pool_.reset(new GpuMemoryBufferVideoFramePool( > What if we have outstanding VideoFrames out there? Will this cause black frames? > > I think NOT because each frame will keep a ref-count to the PoolImpl: > > https://cs.chromium.org/chromium/src/media/video/gpu_memory_buffer_video_fram... > It has been a while but that seems the intended behavior. > But I am not sure :) > > Maybe we should have a GpuMemoryBufferVideoFramePool::Reset() method such that it'll hide these implementation details from it's clients. > Why would clients think of the implementation? They can just delete and create new pool when they want without thinking about VideoFrames lifetime (assuming it works as intended). On the other hand, with a Reset method you'll need to make sure to do the accounting of released buffers correctly when they'll come back after a Reset. I think this solution should work fine. > +dcastagna to double check
On 2017/03/29 01:55:37, servolk wrote: > On 2017/03/27 21:01:27, DaleCurtis wrote: > > Overall lg, but can you clarify the expectations of what is allowed to happen > > between the time a new track is selected and reinitialization completes? What > > happens with calls like playbackRate, seek(), etc? > > Good question. I think playbackRate should be fine and completely unaffected by > track switching (since playback rate is unchanged and known by the RendererImpl > while the switch is happening). But seek() is a different matter. I think > currently seek() might interfere with track switching (since both track > switching and the seek operation involve flushing a/v renderers and we don't > allow more than one pending seek operation at a time). > I've spent a some time today trying to trigger an issue that would cause a > dcheck or a crash when seek occurs while the media track switch is still being > handled. But so far I haven't found a way to trigger. I will continue my > attempts tomorrow. > I believe that the right way to fix this is probably to postpone the seek > operation if it happens when a media track switch is being processed, similar to > how we postpone stream status notification handling if another status change in > being handled (see > https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=9dd...). The easiest way to investigate this is adding RendererImplTests, where we have mock AudioRenderer and VideoRenderer and you can control the exact call sequences :)
On 2017/03/29 01:59:30, Daniele Castagna wrote: > On 2017/03/29 at 00:16:48, xhwang wrote: > > +dcastagna: see my question below > > > > Here's my last round of comments. I think Dale made a good point that we need > to watch out for Renderer calls when we are pending reinitialization. Can you > help check that? Maybe adding more RendererImplTest to cover these cases, and > the new code path? > > > > > https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... > > File media/blink/webmediaplayer_impl.cc (right): > > > > > https://codereview.chromium.org/2684103005/diff/260001/media/blink/webmediapl... > > media/blink/webmediaplayer_impl.cc:1051: /*enabled*/ first_audio_track); > > On 2017/03/27 22:34:12, servolk wrote: > > > On 2017/03/27 19:13:53, xhwang_slow wrote: > > > > This line doesn't make sense now :) > > > > > > If you are talking about the comment, it still does make sense. The comment > > > clarifies that the last parameter is 'bool enabled' and tells the client > whether > > > the added media track should be in the enabled or disabled state initially. > It > > > just so happens that we want only the first track to be enabled initially, > and > > > so we pass the is_first_audio_track in there. Would it be clearer to change > > > comment to something like /* enabled= */ ? > > > > I see. Thanks for the context. I think this style where you put comments > before the variable is very rare and IMHO confusing. > > > > Also, probably add comments about why we only enable/select the first track? > > > > > https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... > > File media/renderers/audio_renderer_impl.h (right): > > > > > https://codereview.chromium.org/2684103005/diff/260001/media/renderers/audio_... > > media/renderers/audio_renderer_impl.h:259: CreateAudioDecodersCB > create_audio_decoders_cb_; > > On 2017/03/27 22:34:13, servolk wrote: > > > On 2017/03/27 19:13:53, xhwang_slow wrote: > > > > FYI, we do have a DecoderFactory interface: > > > > > > > > https://cs.chromium.org/chromium/src/media/base/decoder_factory.h > > > > > > > > I wonder whether it's a good idea to consolidate that with this callback > (in a > > > > later CL) > > > > > > Yes, I saw that, but for now the DefaultRendererFactory doesn't implement > the > > > DecoderFactory interface, so it was easier to get a callback rather than a > full > > > DecoderFactory. Besides, here we only need to create audio decoders (and in > > > video renderer we need to create only video decoders), but in the > DecoderFactory > > > interface these two methods are tied together. I agree that we should > probably > > > revisit this design later. > > > > Could you please add a TODO? > > > > > https://codereview.chromium.org/2684103005/diff/280001/content/renderer/media... > > File content/renderer/media/media_interface_provider.h (right): > > > > > https://codereview.chromium.org/2684103005/diff/280001/content/renderer/media... > > content/renderer/media/media_interface_provider.h:15: #include "url/gurl.h" > > add include for WeakPtr* > > > > > https://codereview.chromium.org/2684103005/diff/280001/content/renderer/media... > > content/renderer/media/media_interface_provider.h:26: // thread where > constructor was called) if necessary. > > That's impl detail. You can probably just say "GetInterface() can be called on > any thread". > > > > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > > File media/renderers/renderer_impl.cc (right): > > > > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > > media/renderers/renderer_impl.cc:267: DVLOG(3) << __func__ << " stream=" << > stream << " time=" << time.InSecondsF(); > > nit: here and everywhere else, media switching isn't very common, and worth a > DVLOG(2) IMHO > > > > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > > media/renderers/renderer_impl.cc:280: void > RendererImpl::OnVideoRendererReinitialized(DemuxerStream* stream, > > nit: Why do we need to pass in the |stream| here and in > RestartVideoRenderer(), given we already set \current_video_stream_\? > > > > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > > File media/renderers/renderer_impl.h (right): > > > > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > > media/renderers/renderer_impl.h:66: base::TimeDelta time); > > This doesn't need to be public. Also it would be nice to be grouped together > with other stream-switching functions, e.g. Reinitialize*Renderer(). > > > > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > > media/renderers/renderer_impl.h:114: // where the switch occured. > > nit: At this level, we are dealing with DemuxerStream swiching (StatusChange) > and we don't need to mention "media track". People looking at this code would > have no idea what a media track is. > > > > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > > media/renderers/renderer_impl.h:125: // media track has been disabled and > re-enabled. The |stream| parameter > > ditto about "media track" > > > > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > > media/renderers/renderer_impl.h:146: BufferingState new_buffering_state); > > empty line > > > > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/render... > > media/renderers/renderer_impl.h:155: BufferingState new_buffering_state); > > empty line > > > > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/video_... > > File media/renderers/video_renderer_impl.cc (right): > > > > > https://codereview.chromium.org/2684103005/diff/280001/media/renderers/video_... > > media/renderers/video_renderer_impl.cc:162: gpu_memory_buffer_pool_.reset(new > GpuMemoryBufferVideoFramePool( > > What if we have outstanding VideoFrames out there? Will this cause black > frames? > > > > I think NOT because each frame will keep a ref-count to the PoolImpl: > > > > > https://cs.chromium.org/chromium/src/media/video/gpu_memory_buffer_video_fram... > > > > It has been a while but that seems the intended behavior. > > > But I am not sure :) > > > > Maybe we should have a GpuMemoryBufferVideoFramePool::Reset() method such that > it'll hide these implementation details from it's clients. > > > > Why would clients think of the implementation? They can just delete and create > new pool when they want without thinking about VideoFrames lifetime (assuming it > works as intended). > On the other hand, with a Reset method you'll need to make sure to do the > accounting of released buffers correctly when they'll come back after a Reset. > I think this solution should work fine. Makes sense. But it wasn't clear to me and Sergey that destructing the pool is safe for outstanding VideoFrames without looking into the implementation details. I though by adding a Reset() call, we can at least have a place to document this. We can have a class level comment for the pool which will also work I guess. Sergey: Could you please add a comment in the GpuMemoryBufferVideoFramePool that destroying the pool will not invalidate or affect outstanding VideoFrames?
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by servolk@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...
On 2017/03/29 05:12:04, xhwang_slow wrote: > On 2017/03/29 01:55:37, servolk wrote: > > On 2017/03/27 21:01:27, DaleCurtis wrote: > > > Overall lg, but can you clarify the expectations of what is allowed to > happen > > > between the time a new track is selected and reinitialization completes? > What > > > happens with calls like playbackRate, seek(), etc? > > > > Good question. I think playbackRate should be fine and completely unaffected > by > > track switching (since playback rate is unchanged and known by the > RendererImpl > > while the switch is happening). But seek() is a different matter. I think > > currently seek() might interfere with track switching (since both track > > switching and the seek operation involve flushing a/v renderers and we don't > > allow more than one pending seek operation at a time). > > I've spent a some time today trying to trigger an issue that would cause a > > dcheck or a crash when seek occurs while the media track switch is still being > > handled. But so far I haven't found a way to trigger. I will continue my > > attempts tomorrow. > > I believe that the right way to fix this is probably to postpone the seek > > operation if it happens when a media track switch is being processed, similar > to > > how we postpone stream status notification handling if another status change > in > > being handled (see > > > https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=9dd...). > > The easiest way to investigate this is adding RendererImplTests, where we have > mock AudioRenderer and VideoRenderer and you can control the exact call > sequences :) Yep, I've ended up adding a couple of new RendererImpl unit tests and making changes to postpone Flush processing until after stream status changes have been handled. PTAL at the latest patchset.
On 2017/03/31 00:29:13, servolk wrote: > On 2017/03/29 05:12:04, xhwang_slow wrote: > > On 2017/03/29 01:55:37, servolk wrote: > > > On 2017/03/27 21:01:27, DaleCurtis wrote: > > > > Overall lg, but can you clarify the expectations of what is allowed to > > happen > > > > between the time a new track is selected and reinitialization completes? > > What > > > > happens with calls like playbackRate, seek(), etc? > > > > > > Good question. I think playbackRate should be fine and completely unaffected > > by > > > track switching (since playback rate is unchanged and known by the > > RendererImpl > > > while the switch is happening). But seek() is a different matter. I think > > > currently seek() might interfere with track switching (since both track > > > switching and the seek operation involve flushing a/v renderers and we don't > > > allow more than one pending seek operation at a time). > > > I've spent a some time today trying to trigger an issue that would cause a > > > dcheck or a crash when seek occurs while the media track switch is still > being > > > handled. But so far I haven't found a way to trigger. I will continue my > > > attempts tomorrow. > > > I believe that the right way to fix this is probably to postpone the seek > > > operation if it happens when a media track switch is being processed, > similar > > to > > > how we postpone stream status notification handling if another status change > > in > > > being handled (see > > > > > > https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=9dd...). > > > > The easiest way to investigate this is adding RendererImplTests, where we have > > mock AudioRenderer and VideoRenderer and you can control the exact call > > sequences :) > > Yep, I've ended up adding a couple of new RendererImpl unit tests and making > changes to postpone Flush processing until after stream status changes have been > handled. PTAL at the latest patchset. Done (added a note in gpu_memory_buffer_video_frame_pool.h)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2684103005/diff/360001/media/video/gpu_memory... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/2684103005/diff/360001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.h:28: // frames. GPU memory buffers are ref-counted and will be kept alive for as long nit: Actually VideoFrames are ref-counted. The resources backing VideoFrames stay alive until all VideoFrames are returned from the compositor. This includes GMBs, images and textures.
https://codereview.chromium.org/2684103005/diff/360001/media/video/gpu_memory... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/2684103005/diff/360001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.h:28: // frames. GPU memory buffers are ref-counted and will be kept alive for as long On 2017/03/31 00:58:13, Daniele Castagna wrote: > nit: Actually VideoFrames are ref-counted. The resources backing VideoFrames > stay alive until all VideoFrames are returned from the compositor. This includes > GMBs, images and textures. Thanks for correcting me. How about this? // NOTE: Destroying the pool will not immediately invalidate outstanding video // frames. GPU memory buffers will be kept alive by video frames referencing // them. Video frames themselves are ref-counted and will be released when they // are no longer needed, potentially after the pool is destroyed.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2684103005/diff/360001/media/video/gpu_memory... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/2684103005/diff/360001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.h:28: // frames. GPU memory buffers are ref-counted and will be kept alive for as long On 2017/03/31 at 01:04:19, servolk wrote: > On 2017/03/31 00:58:13, Daniele Castagna wrote: > > nit: Actually VideoFrames are ref-counted. The resources backing VideoFrames > > stay alive until all VideoFrames are returned from the compositor. This includes > > GMBs, images and textures. > > Thanks for correcting me. How about this? > // NOTE: Destroying the pool will not immediately invalidate outstanding video > // frames. GPU memory buffers will be kept alive by video frames referencing > // them. Video frames themselves are ref-counted and will be released when they > // are no longer needed, potentially after the pool is destroyed. Thanks to you for adding this note. The comment seems good. Maybe just remove the end of the second sentence where you mention that video frames are referencing GMBs (just remove the "referencing them" part), since it might seem they reference them directly. The real story is that the release callbacks of videoframes are keeping alive another ref counted object that contains all the resources needed. But I don't think that belongs to this note. I'm also OK adding an "indirectly" referencing them. Thanks!
https://codereview.chromium.org/2684103005/diff/360001/media/video/gpu_memory... File media/video/gpu_memory_buffer_video_frame_pool.h (right): https://codereview.chromium.org/2684103005/diff/360001/media/video/gpu_memory... media/video/gpu_memory_buffer_video_frame_pool.h:28: // frames. GPU memory buffers are ref-counted and will be kept alive for as long On 2017/03/31 01:20:54, Daniele Castagna wrote: > On 2017/03/31 at 01:04:19, servolk wrote: > > On 2017/03/31 00:58:13, Daniele Castagna wrote: > > > nit: Actually VideoFrames are ref-counted. The resources backing VideoFrames > > > stay alive until all VideoFrames are returned from the compositor. This > includes > > > GMBs, images and textures. > > > > Thanks for correcting me. How about this? > > // NOTE: Destroying the pool will not immediately invalidate outstanding video > > // frames. GPU memory buffers will be kept alive by video frames referencing > > // them. Video frames themselves are ref-counted and will be released when > they > > // are no longer needed, potentially after the pool is destroyed. > > Thanks to you for adding this note. > > The comment seems good. Maybe just remove the end of the second sentence where > you mention that video frames are referencing GMBs (just remove the "referencing > them" part), since it might seem they reference them directly. > > The real story is that the release callbacks of videoframes are keeping alive > another ref counted object that contains all the resources needed. But I don't > think that belongs to this note. > > I'm also OK adding an "indirectly" referencing them. > > Thanks! Done.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/31 01:23:21, servolk wrote: > https://codereview.chromium.org/2684103005/diff/360001/media/video/gpu_memory... > File media/video/gpu_memory_buffer_video_frame_pool.h (right): > > https://codereview.chromium.org/2684103005/diff/360001/media/video/gpu_memory... > media/video/gpu_memory_buffer_video_frame_pool.h:28: // frames. GPU memory > buffers are ref-counted and will be kept alive for as long > On 2017/03/31 01:20:54, Daniele Castagna wrote: > > On 2017/03/31 at 01:04:19, servolk wrote: > > > On 2017/03/31 00:58:13, Daniele Castagna wrote: > > > > nit: Actually VideoFrames are ref-counted. The resources backing > VideoFrames > > > > stay alive until all VideoFrames are returned from the compositor. This > > includes > > > > GMBs, images and textures. > > > > > > Thanks for correcting me. How about this? > > > // NOTE: Destroying the pool will not immediately invalidate outstanding > video > > > // frames. GPU memory buffers will be kept alive by video frames referencing > > > // them. Video frames themselves are ref-counted and will be released when > > they > > > // are no longer needed, potentially after the pool is destroyed. > > > > Thanks to you for adding this note. > > > > The comment seems good. Maybe just remove the end of the second sentence where > > you mention that video frames are referencing GMBs (just remove the > "referencing > > them" part), since it might seem they reference them directly. > > > > The real story is that the release callbacks of videoframes are keeping alive > > another ref counted object that contains all the resources needed. But I don't > > think that belongs to this note. > > > > I'm also OK adding an "indirectly" referencing them. > > > > Thanks! > > Done. Xiaohan/Dale, could you take a look? Do you have any more comments?
Did you ever find anything more about seeking and track changes? The last message says you're continuing to investigate. https://codereview.chromium.org/2684103005/diff/400001/content/renderer/media... File content/renderer/media/media_interface_provider.cc (right): https://codereview.chromium.org/2684103005/diff/400001/content/renderer/media... content/renderer/media/media_interface_provider.cc:20: task_runner_ = base::ThreadTaskRunnerHandle::Get(); No point in having both a task_runner_ and thread_checker_. Just replace ThreadChecker with TaskRunner checks. https://codereview.chromium.org/2684103005/diff/400001/media/renderers/audio_... File media/renderers/audio_renderer_impl_unittest.cc (right): https://codereview.chromium.org/2684103005/diff/400001/media/renderers/audio_... media/renderers/audio_renderer_impl_unittest.cc:483: There needs to be a test for the track switch happening when the AudioRendererImpl is in the middle of a Flush(). https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:237: if ((state_ != STATE_PLAYING) || (audio_ended_ && video_ended_)) Does this prevent the stream status change from occurring while in the middle of a Flush() if not, tests are needed. https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:251: base::Bind((stream == current_video_stream_) Unnecessary parens. https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:267: base::Bind((stream == current_audio_stream_) Ditto. https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:311: video_renderer_->StartPlayingFrom(time); This doesn't seem to toggle OnTimeProgressing? https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl_unittest.cc:858: // Verify that a RendererImpl::Flush gets postponed until after stream status Opposite tests are needed, Flush is ongoing and Reinit occurs?
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:229: void RendererImpl::OnStreamStatusChanged(DemuxerStream* stream, nit: method definition order should match declaration order. But please only do this after you got all approvals. https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:238: return; nit: Add a comment why it's safe to drop this notification when we are in the FLUSHING state. Actually, thinking about this case: 1. We are in the middle of reinit, say, restarting_audio_ is true. And we have 3 pending stream status notifications. 2. Flush() is called, so we enter STATE_FLUSHING state, and return on l.204. 3. Then at OnStreamRestartCompleted(), since pending_stream_status_notifications_ is not empty, we'll process the next pending stream status notification. Then since the state_ isn't PLAYING, we'll return here. Is this possible? I think I am missing something but would like to double check with you. Overall, I feel RendererImpl is getting pretty complicated with the stream switching logic. I wish we can separate that logic out into some helper class but I don't have any concrete idea at this point :( https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:350: audio_renderer_->StartPlaying(); Not for this CL and I think we discussed this before. Can you remind me why we want to start playing here if we have pending stream status change or if we know we are pending flushing? Having to wait until HAVING_ENOUGH to process next stream status change seems unnecessary. https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:713: << " v=" << restarting_video_; nit: it's gonna be hard to understand what a=1 means. Just use the full name like "restarting_audio"? https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:719: pending_stream_status_notifications_.pop_front(); nit: The Run() part calls OnStreamStatusChanged() which touches pending_stream_status_notifications_. We are fine since here restarting_audio_ and restarting_video_ are both false. But it's a bit scary... Can we top, pop_front and then Run(), just to be on the safer side?
https://codereview.chromium.org/2684103005/diff/400001/content/renderer/media... File content/renderer/media/media_interface_provider.cc (right): https://codereview.chromium.org/2684103005/diff/400001/content/renderer/media... content/renderer/media/media_interface_provider.cc:20: task_runner_ = base::ThreadTaskRunnerHandle::Get(); On 2017/03/31 21:22:08, DaleCurtis wrote: > No point in having both a task_runner_ and thread_checker_. Just replace > ThreadChecker with TaskRunner checks. Done. https://codereview.chromium.org/2684103005/diff/400001/media/renderers/audio_... File media/renderers/audio_renderer_impl_unittest.cc (right): https://codereview.chromium.org/2684103005/diff/400001/media/renderers/audio_... media/renderers/audio_renderer_impl_unittest.cc:483: On 2017/03/31 21:22:08, DaleCurtis wrote: > There needs to be a test for the track switch happening when the > AudioRendererImpl is in the middle of a Flush(). Done (in media/renderers/renderer_impl_unittest.cc) https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:229: void RendererImpl::OnStreamStatusChanged(DemuxerStream* stream, On 2017/03/31 23:38:11, xhwang_slow wrote: > nit: method definition order should match declaration order. But please only do > this after you got all approvals. Will do https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:237: if ((state_ != STATE_PLAYING) || (audio_ended_ && video_ended_)) On 2017/03/31 21:22:08, DaleCurtis wrote: > Does this prevent the stream status change from occurring while in the middle of > a Flush() if not, tests are needed. Fixed in the latest patchset (by postponing flush handling via the same action queue as postponed track status changes). https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:238: return; On 2017/03/31 23:38:11, xhwang_slow wrote: > nit: Add a comment why it's safe to drop this notification when we are in the > FLUSHING state. > > Actually, thinking about this case: > > 1. We are in the middle of reinit, say, restarting_audio_ is true. And we have 3 > pending stream status notifications. > 2. Flush() is called, so we enter STATE_FLUSHING state, and return on l.204. > 3. Then at OnStreamRestartCompleted(), since > pending_stream_status_notifications_ is not empty, we'll process the next > pending stream status notification. Then since the state_ isn't PLAYING, we'll > return here. > > Is this possible? I think I am missing something but would like to double check > with you. > > Overall, I feel RendererImpl is getting pretty complicated with the stream > switching logic. I wish we can separate that logic out into some helper class > but I don't have any concrete idea at this point :( Actually was only safe until we allowed track switching. And now that we have track switching we can no longer drop these notifications in the flushing state, instead we'll need to postpone them below (changed in patchset 22). https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:251: base::Bind((stream == current_video_stream_) On 2017/03/31 21:22:08, DaleCurtis wrote: > Unnecessary parens. Done. https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:267: base::Bind((stream == current_audio_stream_) On 2017/03/31 21:22:08, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:311: video_renderer_->StartPlayingFrom(time); On 2017/03/31 21:22:08, DaleCurtis wrote: > This doesn't seem to toggle OnTimeProgressing? Yes, this simply tells video renderer to start reading from the input stream and decoding video frames. When enough video frames are decoded, we'll get OnBufferingStateChange(HAVE_ENOUGH) for the video stream, which will go into HandleRestartedStreamBufferingChanges and, if necessary, will toggle OnTimeProgressing at https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=6f6.... https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:350: audio_renderer_->StartPlaying(); On 2017/03/31 23:38:11, xhwang_slow wrote: > Not for this CL and I think we discussed this before. Can you remind me why we > want to start playing here if we have pending stream status change or if we know > we are pending flushing? Having to wait until HAVING_ENOUGH to process next > stream status change seems unnecessary. Because a. We don't know if the pending stream status change is for the same stream or for a different one and b. If a RendererImpl::Flush is pending, then it'll try to flush AudioRenderer again. Currently AudioRenderer::Flush can be called only from a kPlaying state due to DCHECK at https://cs.chromium.org/chromium/src/media/renderers/audio_renderer_impl.cc?r.... https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:713: << " v=" << restarting_video_; On 2017/03/31 23:38:11, xhwang_slow wrote: > nit: it's gonna be hard to understand what a=1 means. Just use the full name > like "restarting_audio"? Done. https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:719: pending_stream_status_notifications_.pop_front(); On 2017/03/31 23:38:11, xhwang_slow wrote: > nit: The Run() part calls OnStreamStatusChanged() which touches > pending_stream_status_notifications_. We are fine since here restarting_audio_ > and restarting_video_ are both false. But it's a bit scary... > > Can we top, pop_front and then Run(), just to be on the safer side? Done. https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl_unittest.cc:858: // Verify that a RendererImpl::Flush gets postponed until after stream status On 2017/03/31 21:22:08, DaleCurtis wrote: > Opposite tests are needed, Flush is ongoing and Reinit occurs? Done.
Thanks for the tests, just one last question. https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... media/renderers/renderer_impl.cc:210: if (pending_flush_cb_) { Is this necessary? Is something calling with the same flush cb twice? https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... media/renderers/renderer_impl_unittest.cc:1072: } // namespace medi media
https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... media/renderers/renderer_impl.cc:210: if (pending_flush_cb_) { On 2017/04/03 19:17:59, DaleCurtis wrote: > Is this necessary? Is something calling with the same flush cb twice? External clients aren't calling Flush twice. But we can now get to line 210 in two slightly different ways: 1. External caller called Flush and there were no other pending actions, so we are just going to perform Flush right away (pending_flush_cb_ will be empty then). 2. If an external caller calls Flush while we are processing another media track change, then Flush will be postponed (see line 205 above) and it will be reinvoked with the same flush_cb (also saved as pending_flush_cb_ in order to be able to access it in the destructor) after the media track status processing is done. The pending_flush_cb_ is non-null only for case #2. The DCHECK is just a sanity check that we are going to perform the same Flush request that we've previously saved at line 205, and since now we know that there are no other pending actions and we are going to do Flush immediately, we need to reset the |pending_flush_cb_| in order to not call Flush twice if the RendererImpl is destroyed immediately after returning from this function.
https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... media/renderers/renderer_impl_unittest.cc:1072: } // namespace medi On 2017/04/03 19:17:59, DaleCurtis wrote: > media Done.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/400001/media/renderers/render... media/renderers/renderer_impl.cc:229: void RendererImpl::OnStreamStatusChanged(DemuxerStream* stream, On 2017/03/31 23:53:55, servolk wrote: > On 2017/03/31 23:38:11, xhwang_slow wrote: > > nit: method definition order should match declaration order. But please only > do > > this after you got all approvals. > > Will do I've rearranged the methods to match declaration order in the latest patchset, since I have to restart CQ for an comment change anyway :)
lgtm, please wait for xhwang@'s approval too though. https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... media/renderers/renderer_impl.cc:210: if (pending_flush_cb_) { On 2017/04/03 at 20:27:46, servolk wrote: > On 2017/04/03 19:17:59, DaleCurtis wrote: > > Is this necessary? Is something calling with the same flush cb twice? > > External clients aren't calling Flush twice. But we can now get to line 210 in two slightly different ways: > 1. External caller called Flush and there were no other pending actions, so we are just going to perform Flush right away (pending_flush_cb_ will be empty then). > 2. If an external caller calls Flush while we are processing another media track change, then Flush will be postponed (see line 205 above) and it will be reinvoked with the same flush_cb (also saved as pending_flush_cb_ in order to be able to access it in the destructor) after the media track status processing is done. > > The pending_flush_cb_ is non-null only for case #2. The DCHECK is just a sanity check that we are going to perform the same Flush request that we've previously saved at line 205, and since now we know that there are no other pending actions and we are going to do Flush immediately, we need to reset the |pending_flush_cb_| in order to not call Flush twice if the RendererImpl is destroyed immediately after returning from this function. Ah, in that case I think this should call something like FlushInternal() which doesn't set flush_cb_. It'd be clearer if this was a base::OnceCallback() but that can be done another time.
https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... media/renderers/renderer_impl.cc:210: if (pending_flush_cb_) { On 2017/04/03 21:32:41, DaleCurtis wrote: > On 2017/04/03 at 20:27:46, servolk wrote: > > On 2017/04/03 19:17:59, DaleCurtis wrote: > > > Is this necessary? Is something calling with the same flush cb twice? > > > > External clients aren't calling Flush twice. But we can now get to line 210 in > two slightly different ways: > > 1. External caller called Flush and there were no other pending actions, so we > are just going to perform Flush right away (pending_flush_cb_ will be empty > then). > > 2. If an external caller calls Flush while we are processing another media > track change, then Flush will be postponed (see line 205 above) and it will be > reinvoked with the same flush_cb (also saved as pending_flush_cb_ in order to be > able to access it in the destructor) after the media track status processing is > done. > > > > The pending_flush_cb_ is non-null only for case #2. The DCHECK is just a > sanity check that we are going to perform the same Flush request that we've > previously saved at line 205, and since now we know that there are no other > pending actions and we are going to do Flush immediately, we need to reset the > |pending_flush_cb_| in order to not call Flush twice if the RendererImpl is > destroyed immediately after returning from this function. > > Ah, in that case I think this should call something like FlushInternal() which > doesn't set flush_cb_. It'd be clearer if this was a base::OnceCallback() but > that can be done another time. Wait, I don't follow. Where would we set flush_cb_ then and what would the FlushInternal be like - just the lines 218-221 below?
https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... media/renderers/renderer_impl.cc:210: if (pending_flush_cb_) { On 2017/04/03 at 22:38:33, servolk wrote: > On 2017/04/03 21:32:41, DaleCurtis wrote: > > On 2017/04/03 at 20:27:46, servolk wrote: > > > On 2017/04/03 19:17:59, DaleCurtis wrote: > > > > Is this necessary? Is something calling with the same flush cb twice? > > > > > > External clients aren't calling Flush twice. But we can now get to line 210 in > > two slightly different ways: > > > 1. External caller called Flush and there were no other pending actions, so we > > are just going to perform Flush right away (pending_flush_cb_ will be empty > > then). > > > 2. If an external caller calls Flush while we are processing another media > > track change, then Flush will be postponed (see line 205 above) and it will be > > reinvoked with the same flush_cb (also saved as pending_flush_cb_ in order to be > > able to access it in the destructor) after the media track status processing is > > done. > > > > > > The pending_flush_cb_ is non-null only for case #2. The DCHECK is just a > > sanity check that we are going to perform the same Flush request that we've > > previously saved at line 205, and since now we know that there are no other > > pending actions and we are going to do Flush immediately, we need to reset the > > |pending_flush_cb_| in order to not call Flush twice if the RendererImpl is > > destroyed immediately after returning from this function. > > > > Ah, in that case I think this should call something like FlushInternal() which > > doesn't set flush_cb_. It'd be clearer if this was a base::OnceCallback() but > > that can be done another time. > > Wait, I don't follow. Where would we set flush_cb_ then and what would the FlushInternal be like - just the lines 218-221 below? I was first thinking you'd post to a Flush that doesn't set the cb, but then I was thinking you'd never save pending_flush_cb_ and just post Flush() without saving it. But actually (some more thinking!), the renderers and decoders are already flushed after a track switch, so why not just clear and run pending_flush_cb_ when the switch completes?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/440001/media/renderers/render... media/renderers/renderer_impl.cc:210: if (pending_flush_cb_) { On 2017/04/03 22:50:35, DaleCurtis wrote: > On 2017/04/03 at 22:38:33, servolk wrote: > > On 2017/04/03 21:32:41, DaleCurtis wrote: > > > On 2017/04/03 at 20:27:46, servolk wrote: > > > > On 2017/04/03 19:17:59, DaleCurtis wrote: > > > > > Is this necessary? Is something calling with the same flush cb twice? > > > > > > > > External clients aren't calling Flush twice. But we can now get to line > 210 in > > > two slightly different ways: > > > > 1. External caller called Flush and there were no other pending actions, > so we > > > are just going to perform Flush right away (pending_flush_cb_ will be empty > > > then). > > > > 2. If an external caller calls Flush while we are processing another media > > > track change, then Flush will be postponed (see line 205 above) and it will > be > > > reinvoked with the same flush_cb (also saved as pending_flush_cb_ in order > to be > > > able to access it in the destructor) after the media track status processing > is > > > done. > > > > > > > > The pending_flush_cb_ is non-null only for case #2. The DCHECK is just a > > > sanity check that we are going to perform the same Flush request that we've > > > previously saved at line 205, and since now we know that there are no other > > > pending actions and we are going to do Flush immediately, we need to reset > the > > > |pending_flush_cb_| in order to not call Flush twice if the RendererImpl is > > > destroyed immediately after returning from this function. > > > > > > Ah, in that case I think this should call something like FlushInternal() > which > > > doesn't set flush_cb_. It'd be clearer if this was a base::OnceCallback() > but > > > that can be done another time. > > > > Wait, I don't follow. Where would we set flush_cb_ then and what would the > FlushInternal be like - just the lines 218-221 below? > > I was first thinking you'd post to a Flush that doesn't set the cb, but then I > was thinking you'd never save pending_flush_cb_ and just post Flush() without > saving it. But actually (some more thinking!), the renderers and decoders are > already flushed after a track switch, so why not just clear and run > pending_flush_cb_ when the switch completes? What I think you missed here is the fact that only ONE of renderers will be in a 'recently flushed' state after the track switch, but the Flush needs to ensure BOTH audio AND video renderer are flushed. So we still need to flush the other one. And initially I wanted to avoid modifying the existing code in the RendererImpl::FlushAudio/VideoRenderer, that's why I did it like this. We just had a discussion about this with Xiaohan, we'll see if we can simplify this logic.
I chat with Sergey offline. The current CL is mostly working but it's getting kinda complicated, with probably a few corner cases missing. We'll take a little bit more time trying to see whether we can get anything simpler. Also, looking at the current RendererImpl. The STATE_PLAYING is a bit misleading now. It really contains two states: 1. FLUSHED: freshly initialized or flushed, but before StartPlayingFrom is called. 2. PLAYING: StartPlayingFrom has been called. For (1), after stream switch, we should not call StartPlayingFrom(). But for (2), we should. The other tricky case is when RendererImpl is in FLUSHED state (StartPlayingFrom() not called), and we get stream switch. Then during stream switch, we get StartPlayingFrom() call. In that case, we should probably postpone the StartPlayingFrom() call as well. The current StartPlayingFrom() call is not checking stream switch status at all, but in theory we could be in the reinitializing state.
On 2017/04/04 00:10:15, xhwang_slow wrote: > I chat with Sergey offline. The current CL is mostly working but it's getting > kinda complicated, with probably a few corner cases missing. We'll take a little > bit more time trying to see whether we can get anything simpler. > > Also, looking at the current RendererImpl. The STATE_PLAYING is a bit misleading > now. It really contains two states: > 1. FLUSHED: freshly initialized or flushed, but before StartPlayingFrom is > called. > 2. PLAYING: StartPlayingFrom has been called. > For (1), after stream switch, we should not call StartPlayingFrom(). But for > (2), we should. > > The other tricky case is when RendererImpl is in FLUSHED state > (StartPlayingFrom() not called), and we get stream switch. Then during stream > switch, we get StartPlayingFrom() call. In that case, we should probably > postpone the StartPlayingFrom() call as well. The current StartPlayingFrom() > call is not checking stream switch status at all, but in theory we could be in > the reinitializing state. I didn't have any better ideas at this point. So let's fix the tricky cases mentioned above (potentially by adding a new state in a separate CL), and we should be good to go. We can always revisit the solution if we have any new ideas.
On 2017/04/05 06:29:47, xhwang_slow wrote: > On 2017/04/04 00:10:15, xhwang_slow wrote: > > I chat with Sergey offline. The current CL is mostly working but it's getting > > kinda complicated, with probably a few corner cases missing. We'll take a > little > > bit more time trying to see whether we can get anything simpler. > > > > Also, looking at the current RendererImpl. The STATE_PLAYING is a bit > misleading > > now. It really contains two states: > > 1. FLUSHED: freshly initialized or flushed, but before StartPlayingFrom is > > called. > > 2. PLAYING: StartPlayingFrom has been called. > > For (1), after stream switch, we should not call StartPlayingFrom(). But for > > (2), we should. > > > > The other tricky case is when RendererImpl is in FLUSHED state > > (StartPlayingFrom() not called), and we get stream switch. Then during stream > > switch, we get StartPlayingFrom() call. In that case, we should probably > > postpone the StartPlayingFrom() call as well. The current StartPlayingFrom() > > call is not checking stream switch status at all, but in theory we could be in > > the reinitializing state. > > I didn't have any better ideas at this point. So let's fix the tricky cases > mentioned above (potentially by adding a new state in a separate CL), and we > should be good to go. We can always revisit the solution if we have any new > ideas. Thanks Xiaohan! I've also spent some time yesterday thinking about this and put together a couple of CLs on top of this one: 1. Add an intermediate FLUSHED state to the renderer: https://codereview.chromium.org/2804493002/ Renderer will be in the FLUSHED state after Flush completes, but before StartPlayingFrom is called. Having this new state will allow us to decide if we need to Flush before the track switch, or skip the Flush, since the renderers are already flushed. 2. Now we only need to care about 3 states in the OnStreamStatusChanged. If the renderer is in the FLUSHING state, we'll simply postpone the stream status handling. If the renderer is in the FLUSHED state, we can switch tracks (reinitialize a/v renderer) right away, without performing a flush on sub-renderer first. And if the renderer is in the PLAYING state, then we'll do the usual track switch (i.e. Flush the corresponding sub-renderer and Reinitialize or Restart it as necessary): https://codereview.chromium.org/2800523003/ The unit tests added in this CL (Audio/VideoTrackSwitchDuringFlush) already provide test coverage for this, we'll only need to adjust test expectations to not expect sub-renderer Flush/StartPlaying(From) to happen when a track switch is completed in the FLUSHED state.
The CQ bit was checked by servolk@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@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...
On 2017/04/05 16:58:39, servolk wrote: > On 2017/04/05 06:29:47, xhwang_slow wrote: > > On 2017/04/04 00:10:15, xhwang_slow wrote: > > > I chat with Sergey offline. The current CL is mostly working but it's > getting > > > kinda complicated, with probably a few corner cases missing. We'll take a > > little > > > bit more time trying to see whether we can get anything simpler. > > > > > > Also, looking at the current RendererImpl. The STATE_PLAYING is a bit > > misleading > > > now. It really contains two states: > > > 1. FLUSHED: freshly initialized or flushed, but before StartPlayingFrom is > > > called. > > > 2. PLAYING: StartPlayingFrom has been called. > > > For (1), after stream switch, we should not call StartPlayingFrom(). But for > > > (2), we should. > > > > > > The other tricky case is when RendererImpl is in FLUSHED state > > > (StartPlayingFrom() not called), and we get stream switch. Then during > stream > > > switch, we get StartPlayingFrom() call. In that case, we should probably > > > postpone the StartPlayingFrom() call as well. The current StartPlayingFrom() > > > call is not checking stream switch status at all, but in theory we could be > in > > > the reinitializing state. > > > > I didn't have any better ideas at this point. So let's fix the tricky cases > > mentioned above (potentially by adding a new state in a separate CL), and we > > should be good to go. We can always revisit the solution if we have any new > > ideas. > > Thanks Xiaohan! I've also spent some time yesterday thinking about this and put > together a couple of CLs on top of this one: > 1. Add an intermediate FLUSHED state to the renderer: > https://codereview.chromium.org/2804493002/ > Renderer will be in the FLUSHED state after Flush completes, but before > StartPlayingFrom is called. Having this new state will allow us to decide if we > need to Flush before the track switch, or skip the Flush, since the renderers > are already flushed. > > 2. Now we only need to care about 3 states in the OnStreamStatusChanged. If the > renderer is in the FLUSHING state, we'll simply postpone the stream status > handling. If the renderer is in the FLUSHED state, we can switch tracks > (reinitialize a/v renderer) right away, without performing a flush on > sub-renderer first. And if the renderer is in the PLAYING state, then we'll do > the usual track switch (i.e. Flush the corresponding sub-renderer and > Reinitialize or Restart it as necessary): > https://codereview.chromium.org/2800523003/ > > The unit tests added in this CL (Audio/VideoTrackSwitchDuringFlush) already > provide test coverage for this, we'll only need to adjust test expectations to > not expect sub-renderer Flush/StartPlaying(From) to happen when a track switch > is completed in the FLUSHED state. Note: I went ahead and merged https://codereview.chromium.org/2800523003/ into this CL, since these changes are necessary to fix test failures seen in patchset #26.
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 servolk@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.
looking good. just two questions! https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... media/renderers/renderer_impl.cc:211: base::Bind(&RendererImpl::Flush, weak_this_, pending_flush_cb_)); Can you just use base::Bind(&RendererImpl::Flush, weak_this_, flush_cb) here so we don't need pending_flush_cb_? https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... media/renderers/renderer_impl.cc:664: audio_renderer_->StartPlaying(); nit: add a comment when stream restart will be finished in this case https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... media/renderers/renderer_impl_unittest.cc:1054: } Can you add test cases for: - stream switch in "FLUSHED" state
https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... media/renderers/renderer_impl.cc:211: base::Bind(&RendererImpl::Flush, weak_this_, pending_flush_cb_)); On 2017/04/10 18:57:30, xhwang_slow wrote: > Can you just use > > base::Bind(&RendererImpl::Flush, weak_this_, flush_cb) > > here so we don't need pending_flush_cb_? We could remove the |pending_flush_cb_| and use |flush_cb_| instead, but then we'd have to remove the DCHECK at line 189 above, since flush_cb_ would be non-null when restarting the postponed Flush after a track switch. But that DCHECK is also the only DCHECK that prevents Flush from being called while a previous Flush is pending, so I'm not sure if that's a good idea. I think leaving pending_flush_cb_ to keep a clear separation between the regular and postponed Flush is actually better, even though it's an extra variable. WDYT? https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... media/renderers/renderer_impl.cc:664: audio_renderer_->StartPlaying(); On 2017/04/10 18:57:30, xhwang_slow wrote: > nit: add a comment when stream restart will be finished in this case Done. https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... media/renderers/renderer_impl_unittest.cc:1054: } On 2017/04/10 18:57:30, xhwang_slow wrote: > Can you add test cases for: > - stream switch in "FLUSHED" state The *TrackSwitchDuringFlush actually include testing of both states. For example in VideoTrackSwitchDuringFlush we do the track switch on lines 1042-1043, while the renderer is still in the FLUSHING state (due to the renderer_impl_->Flush invocation at line 1034 and due to a pending video_renderer_flush_cb). So the track switch gets done in the FLUSHING state and postponed initially. Later, when we complete the video renderer flush on line 1052, it allows the pending Flush from line 1034 to complete and triggers processing of the pending track status changes. Now the pending status changes initiated at lines 1042-1043 will run again, but this time the renderer_impl_ is in the FLUSHED state, which allow the stream switch to proceed and call the video_renderer_ Initialize with the secondary video stream satisfying the EXPECT_CALL at line 1048.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... media/renderers/renderer_impl.cc:211: base::Bind(&RendererImpl::Flush, weak_this_, pending_flush_cb_)); On 2017/04/10 20:21:22, servolk wrote: > On 2017/04/10 18:57:30, xhwang_slow wrote: > > Can you just use > > > > base::Bind(&RendererImpl::Flush, weak_this_, flush_cb) > > > > here so we don't need pending_flush_cb_? > > We could remove the |pending_flush_cb_| and use |flush_cb_| instead, but then > we'd have to remove the DCHECK at line 189 above, since flush_cb_ would be > non-null when restarting the postponed Flush after a track switch. But that > DCHECK is also the only DCHECK that prevents Flush from being called while a > previous Flush is pending, so I'm not sure if that's a good idea. I think > leaving pending_flush_cb_ to keep a clear separation between the regular and > postponed Flush is actually better, even though it's an extra variable. WDYT? Actually, let me expand a bit more. The main reason why I added the pending_flush_cb_ is that I wanted to preserve the guarantee that the flush_cb always gets invoked, even if the RendererImpl is destroyed before the flush completes. Imagine a situation like: 1. media track switch happens, the RendererImpl starts asynchronously processing it, but it might take some time. 2. RendererImpl::Flush is called. The Flush will be postponed due to a pending audio/video restart due to a track switch in #1. When the Flush is postponed, it gets placed into the pending_actions_ queue. 3. Now RendererImpl gets destroyed, before the track switch and Flush are completed. How can we guarantee that the flush_cb gets invoked in this case? We probably don't want to go and execute pending_actions_ queue, since that's useless and will only add to destruction delay. We need to know in the destructor whether there was a pending flush, whose cb we need to invoke, and we need to be able to invoke the cb without going through the whole pending_actions_ queue. I believe the easiest solution to that is to store a flush_cb in a pending_flush_cb_ as soon as we put the pending flush in the pending_actions_ queue. If we stored the pending flush callback in the flush_cb_ even for postponed flushes, then we would run into the problem with the DCHECK on line 189.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... media/renderers/renderer_impl.cc:211: base::Bind(&RendererImpl::Flush, weak_this_, pending_flush_cb_)); On 2017/04/10 20:29:53, servolk wrote: > On 2017/04/10 20:21:22, servolk wrote: > > On 2017/04/10 18:57:30, xhwang_slow wrote: > > > Can you just use > > > > > > base::Bind(&RendererImpl::Flush, weak_this_, flush_cb) > > > > > > here so we don't need pending_flush_cb_? > > > > We could remove the |pending_flush_cb_| and use |flush_cb_| instead, but then > > we'd have to remove the DCHECK at line 189 above, since flush_cb_ would be > > non-null when restarting the postponed Flush after a track switch. I am confused. We are not setting the |flush_cb_| here yet. Why do we need to remove the DCHECK on l.189? > > But that > > DCHECK is also the only DCHECK that prevents Flush from being called while a > > previous Flush is pending, so I'm not sure if that's a good idea. I think > > leaving pending_flush_cb_ to keep a clear separation between the regular and > > postponed Flush is actually better, even though it's an extra variable. WDYT? > > Actually, let me expand a bit more. The main reason why I added the > pending_flush_cb_ is that I wanted to preserve the guarantee that the flush_cb > always gets invoked, even if the RendererImpl is destroyed before the flush > completes. Imagine a situation like: > 1. media track switch happens, the RendererImpl starts asynchronously > processing it, but it might take some time. > 2. RendererImpl::Flush is called. The Flush will be postponed due to a pending > audio/video restart due to a track switch in #1. When the Flush is postponed, it > gets placed into the pending_actions_ queue. > 3. Now RendererImpl gets destroyed, before the track switch and Flush are > completed. How can we guarantee that the flush_cb gets invoked in this case? We > probably don't want to go and execute pending_actions_ queue, since that's > useless and will only add to destruction delay. We need to know in the > destructor whether there was a pending flush, whose cb we need to invoke, and we > need to be able to invoke the cb without going through the whole > pending_actions_ queue. > I believe the easiest solution to that is to store a flush_cb in a > pending_flush_cb_ as soon as we put the pending flush in the pending_actions_ > queue. > If we stored the pending flush callback in the flush_cb_ even for postponed > flushes, then we would run into the problem with the DCHECK on line 189. It seems this is another case we need https://groups.google.com/a/chromium.org/d/msg/chromium-dev/ZfL_XhGZ7tA/Qrzw9... :) I see your concern, and it makes sense for the current media::Renderer API: https://cs.chromium.org/chromium/src/media/base/renderer.h?rcl=4a485532d97f66... But looking at our implementation in PipelineWrapper, I guess it doesn't make any difference: https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=4a485532... So I feel there's room to improve here (in later CLs). So my last comment for this CL :) Why the FlushInternal() idea Dale mentioned won't work. I think the ideas is: void RendererImpl::Flush(const base::Closure& flush_cb) { ... flush_cb_ = flush_cb; state_ = STATE_FLUSHING; if (restarting_audio_ || restarting_video_) { pending_actions_.push_back( base::Bind(&RendererImpl::FlushInternal, weak_this_)); return; } FlushInternal(); } void RendererImpl::FlushInternal() { // Check for STATE_ERROR etc DCHECK(flush_cb_); DCHECK_EQ(STATE_FLUSHING, state_); if (time_ticking_) PausePlayback(); FlushAudioRenderer(); } https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... media/renderers/renderer_impl_unittest.cc:1054: } On 2017/04/10 20:21:22, servolk wrote: > On 2017/04/10 18:57:30, xhwang_slow wrote: > > Can you add test cases for: > > - stream switch in "FLUSHED" state > > The *TrackSwitchDuringFlush actually include testing of both states. For example > in VideoTrackSwitchDuringFlush we do the track switch on lines 1042-1043, while > the renderer is still in the FLUSHING state (due to the renderer_impl_->Flush > invocation at line 1034 and due to a pending video_renderer_flush_cb). So the > track switch gets done in the FLUSHING state and postponed initially. Later, > when we complete the video renderer flush on line 1052, it allows the pending > Flush from line 1034 to complete and triggers processing of the pending track > status changes. Now the pending status changes initiated at lines 1042-1043 will > run again, but this time the renderer_impl_ is in the FLUSHED state, which allow > the stream switch to proceed and call the video_renderer_ Initialize with the > secondary video stream satisfying the EXPECT_CALL at line 1048. Thanks, this is not obvious from the test name. Could you please add a comment?
https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... media/renderers/renderer_impl.cc:211: base::Bind(&RendererImpl::Flush, weak_this_, pending_flush_cb_)); On 2017/04/11 00:29:41, xhwang_slow wrote: > On 2017/04/10 20:29:53, servolk wrote: > > On 2017/04/10 20:21:22, servolk wrote: > > > On 2017/04/10 18:57:30, xhwang_slow wrote: > > > > Can you just use > > > > > > > > base::Bind(&RendererImpl::Flush, weak_this_, flush_cb) > > > > > > > > here so we don't need pending_flush_cb_? > > > > > > We could remove the |pending_flush_cb_| and use |flush_cb_| instead, but > then > > > we'd have to remove the DCHECK at line 189 above, since flush_cb_ would be > > > non-null when restarting the postponed Flush after a track switch. > > I am confused. We are not setting the |flush_cb_| here yet. Why do we need to > remove the DCHECK on l.189? > > > > But that > > > DCHECK is also the only DCHECK that prevents Flush from being called while a > > > previous Flush is pending, so I'm not sure if that's a good idea. I think > > > leaving pending_flush_cb_ to keep a clear separation between the regular and > > > postponed Flush is actually better, even though it's an extra variable. > WDYT? > > > > Actually, let me expand a bit more. The main reason why I added the > > pending_flush_cb_ is that I wanted to preserve the guarantee that the flush_cb > > always gets invoked, even if the RendererImpl is destroyed before the flush > > completes. Imagine a situation like: > > 1. media track switch happens, the RendererImpl starts asynchronously > > processing it, but it might take some time. > > 2. RendererImpl::Flush is called. The Flush will be postponed due to a pending > > audio/video restart due to a track switch in #1. When the Flush is postponed, > it > > gets placed into the pending_actions_ queue. > > 3. Now RendererImpl gets destroyed, before the track switch and Flush are > > completed. How can we guarantee that the flush_cb gets invoked in this case? > We > > probably don't want to go and execute pending_actions_ queue, since that's > > useless and will only add to destruction delay. We need to know in the > > destructor whether there was a pending flush, whose cb we need to invoke, and > we > > need to be able to invoke the cb without going through the whole > > pending_actions_ queue. > > I believe the easiest solution to that is to store a flush_cb in a > > pending_flush_cb_ as soon as we put the pending flush in the pending_actions_ > > queue. > > If we stored the pending flush callback in the flush_cb_ even for postponed > > flushes, then we would run into the problem with the DCHECK on line 189. > > It seems this is another case we need > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/ZfL_XhGZ7tA/Qrzw9... > :) > > I see your concern, and it makes sense for the current media::Renderer API: > https://cs.chromium.org/chromium/src/media/base/renderer.h?rcl=4a485532d97f66... > > But looking at our implementation in PipelineWrapper, I guess it doesn't make > any difference: > https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=4a485532... > > So I feel there's room to improve here (in later CLs). > > So my last comment for this CL :) Why the FlushInternal() idea Dale mentioned > won't work. I think the ideas is: > > void RendererImpl::Flush(const base::Closure& flush_cb) { > ... > > flush_cb_ = flush_cb; > state_ = STATE_FLUSHING; > > if (restarting_audio_ || restarting_video_) { > pending_actions_.push_back( > base::Bind(&RendererImpl::FlushInternal, weak_this_)); > return; > } > > FlushInternal(); > } > > void RendererImpl::FlushInternal() { > // Check for STATE_ERROR etc > > DCHECK(flush_cb_); > DCHECK_EQ(STATE_FLUSHING, state_); > > if (time_ticking_) > PausePlayback(); > > FlushAudioRenderer(); > } Well, yes, if we do it like that (by setting state_ to STATE_FLUSHING immediately, instead of after the FlushInternal actually starts) then we can keep the DCHECK. We'll just need to make some small adjustments to the RestartAudio/VideoRenderer logic (since now those might get invoked when we are in the STATE_FLUSHING state). Done in the latest patchset, PTAL. https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2684103005/diff/520001/media/renderers/render... media/renderers/renderer_impl_unittest.cc:1054: } On 2017/04/11 00:29:41, xhwang_slow wrote: > On 2017/04/10 20:21:22, servolk wrote: > > On 2017/04/10 18:57:30, xhwang_slow wrote: > > > Can you add test cases for: > > > - stream switch in "FLUSHED" state > > > > The *TrackSwitchDuringFlush actually include testing of both states. For > example > > in VideoTrackSwitchDuringFlush we do the track switch on lines 1042-1043, > while > > the renderer is still in the FLUSHING state (due to the renderer_impl_->Flush > > invocation at line 1034 and due to a pending video_renderer_flush_cb). So the > > track switch gets done in the FLUSHING state and postponed initially. Later, > > when we complete the video renderer flush on line 1052, it allows the pending > > Flush from line 1034 to complete and triggers processing of the pending track > > status changes. Now the pending status changes initiated at lines 1042-1043 > will > > run again, but this time the renderer_impl_ is in the FLUSHED state, which > allow > > the stream switch to proceed and call the video_renderer_ Initialize with the > > secondary video stream satisfying the EXPECT_CALL at line 1048. > > Thanks, this is not obvious from the test name. Could you please add a comment? Done.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by servolk@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, thank you for the patience! :)
On 2017/04/11 16:47:06, xhwang_slow wrote: > LGTM, thank you for the patience! :) Thank you and Dale for the thorough review! :)
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2684103005/#ps590001 (title: "Fixed comments")
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": 590001, "attempt_start_ts": 1491929618587950,
"parent_rev": "fbb8b20231db093fffa5b3a361bac806c1920e82", "commit_rev":
"16e8bdf82977fa9734263248666543e204a930a4"}
Message was sent while issue was closed.
Description was changed from
==========
Allow media track switching.
This CL allows Audio and VideoRenderer to be reinitialized in the
flushed/stopped states and allows switching between different audio
and video tracks during playback (but only a single track of each type
may be active at a time).
BUG=487288
==========
to
==========
Allow media track switching.
This CL allows Audio and VideoRenderer to be reinitialized in the
flushed/stopped states and allows switching between different audio
and video tracks during playback (but only a single track of each type
may be active at a time).
BUG=487288
Review-Url: https://codereview.chromium.org/2684103005
Cr-Commit-Position: refs/heads/master@{#463658}
Committed:
https://chromium.googlesource.com/chromium/src/+/16e8bdf82977fa97342632486665...
==========
Message was sent while issue was closed.
Committed patchset #31 (id:590001) as https://chromium.googlesource.com/chromium/src/+/16e8bdf82977fa97342632486665... |
