Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2558213002:
Fix crash in renderer initialization when media pipeline is stopped (Closed)
DescriptionFix crash in renderer initialization when media pipeline is stopped
The crash happens because by the time we call Demuxer::GetStream again
the main thread might have proceeded with destroying SourceBuffer and
thus ChunkDemuxer no longer allows accessing demuxer streams. Luckily
demuxer streams themselves are still alive, just moved to
ChunkDemuxer::removed_streams_ (see ChunkDemuxer::RemoveId). So cached
pointers can still be used safely.
BUG=668604
Committed: https://crrev.com/ee02c5e1edfa2bfb94d115aa985db582bcf20d2e
Cr-Commit-Position: refs/heads/master@{#438000}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Add test expectations for DemuxStreamProvider GetStream #
Total comments: 10
Patch Set 3 : Move SetStreamStatusChangeCB invokations to a/v renderer init #
Total comments: 4
Patch Set 4 : Added a comment for DemuxerStreamProvider::GetStream #
Dependent Patchsets: Messages
Total messages: 57 (23 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 ========== Fix crash in renderer initialization when media pipeline is stopped The crash happens because by the time we call Demuxer::GetStream again the main thread might have proceeded with destroying SourceBuffer and thus ChunkDemuxer no longer allows accessing demuxer streams. Luckily demuxer streams themselves are still alive, just moved to ChunkDemuxer::removed_streams_ (see ChunkDemuxer::RemoveId). So cached pointers can still be used safely. BUG=668604 ========== to ========== Fix crash in renderer initialization when media pipeline is stopped The crash happens because by the time we call Demuxer::GetStream again the main thread might have proceeded with destroying SourceBuffer and thus ChunkDemuxer no longer allows accessing demuxer streams. Luckily demuxer streams themselves are still alive, just moved to ChunkDemuxer::removed_streams_ (see ChunkDemuxer::RemoveId). So cached pointers can still be used safely. BUG=668604 ==========
servolk@chromium.org changed reviewers: + dalecurtis@chromium.org, wolenetz@chromium.org
On 2016/12/07 23:44:50, servolk wrote: > mailto:servolk@chromium.org changed reviewers: > + mailto:dalecurtis@chromium.org, mailto:wolenetz@chromium.org See https://bugs.chromium.org/p/chromium/issues/detail?id=668604#c11 and https://bugs.chromium.org/p/chromium/issues/detail?id=668604#c12 for a bit more context.
lgtm
Can you create a test where GetStream() returns nullptr the second time?
On 2016/12/08 00:13:34, DaleCurtis wrote: > Can you create a test where GetStream() returns nullptr the second time? I was thinking about how we could create a test for this and was looking at WebMediaPlayerImplTest hoping to find a way, but no luck so far. The difficulty here is that this is a race between the media thread running RendererImpl::InitializeVideoRenderer and the main thread running HTMLMediaElement::resetMediaPlayerAndMediaSource. GetStream will return valid stream pointers until MediaSource is closed and will return nullptrs after that. But so far I haven't found any hooks that would allow us to catch the right moment on the media thread when the MediaSource is already closed, but WMPI is not destroyed yet (and after WMPI is destroyed, the ChunkDemuxer is destroyed with it too, so we can no longer call GetStream). I'll give it some more thought.
the renderer_impl tests have mocking for GetStream() already. Should be fairly trivial to setup this test there. It serves as a good unit test even if it doesn't test the exact bug you're trying to fix here.
On 2016/12/08 00:33:45, DaleCurtis wrote: > the renderer_impl tests have mocking for GetStream() already. Should be fairly > trivial to setup this test there. It serves as a good unit test even if it > doesn't test the exact bug you're trying to fix here. Wait, we seem to be talking about different things. I was thinking about creating the test along the lines: 1. Initialize WMPI and verify that Demuxer::GetStream returns a valid stream. 2. Emulate MediaSource/SourceBuffer being destroyed for WMPI and verify that Demuxer::GetStream returns null after that. You are talking about renderer_impl test, but it's not clear what we would test there. After this change we will no longer call GetStream twice for each stream type, so what could we verify in there? Sure we could set up the GetStream mock to return the valid stream only the first time and return null on subsequent invocations. But GetStream should only get invoked once per a/v renderer init after this change. I guess we could enforce that GetStream mock is only invoked once for each stream type during renderer init - is that what you meant?
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: This issue passed the CQ dry run.
On 2016/12/08 00:51:43, servolk wrote: > On 2016/12/08 00:33:45, DaleCurtis wrote: > > the renderer_impl tests have mocking for GetStream() already. Should be fairly > > trivial to setup this test there. It serves as a good unit test even if it > > doesn't test the exact bug you're trying to fix here. > > Wait, we seem to be talking about different things. I was thinking about > creating the test along the lines: > 1. Initialize WMPI and verify that Demuxer::GetStream returns a valid stream. > 2. Emulate MediaSource/SourceBuffer being destroyed for WMPI and verify that > Demuxer::GetStream returns null after that. > > You are talking about renderer_impl test, but it's not clear what we would test > there. After this change we will no longer call GetStream twice for each stream > type, so what could we verify in there? Sure we could set up the GetStream mock > to return the valid stream only the first time and return null on subsequent > invocations. But GetStream should only get invoked once per a/v renderer init > after this change. I guess we could enforce that GetStream mock is only invoked > once for each stream type during renderer init - is that what you meant? Btw, we already have checks that Demuxer::GetStream returns nulls after ChunkDemuxer::RemoveId, see https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer_unittest.cc?...
On 2016/12/08 at 02:44:06, servolk wrote: > On 2016/12/08 00:51:43, servolk wrote: > > On 2016/12/08 00:33:45, DaleCurtis wrote: > > > the renderer_impl tests have mocking for GetStream() already. Should be fairly > > > trivial to setup this test there. It serves as a good unit test even if it > > > doesn't test the exact bug you're trying to fix here. > > > > Wait, we seem to be talking about different things. I was thinking about > > creating the test along the lines: > > 1. Initialize WMPI and verify that Demuxer::GetStream returns a valid stream. > > 2. Emulate MediaSource/SourceBuffer being destroyed for WMPI and verify that > > Demuxer::GetStream returns null after that. > > > > You are talking about renderer_impl test, but it's not clear what we would test > > there. After this change we will no longer call GetStream twice for each stream > > type, so what could we verify in there? Sure we could set up the GetStream mock > > to return the valid stream only the first time and return null on subsequent > > invocations. But GetStream should only get invoked once per a/v renderer init > > after this change. I guess we could enforce that GetStream mock is only invoked > > once for each stream type during renderer init - is that what you meant? > > Btw, we already have checks that Demuxer::GetStream returns nulls after ChunkDemuxer::RemoveId, see https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer_unittest.cc?... Just having a test which fails on a second GetStream or returns null is valid and recommended here. It'll help catch future races.
https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:134: DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) || It looks like this DCHECK might also fail if pipeline stops fast enough that we lose the race here. https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:143: if (audio_stream) Does it matter that we're setting up these CB's here on a stream that we might not actually use (since GetStream in Initialize{Audio,Video}Renderer later might return null)?
On 2016/12/08 18:08:37, DaleCurtis wrote: > On 2016/12/08 at 02:44:06, servolk wrote: > > On 2016/12/08 00:51:43, servolk wrote: > > > On 2016/12/08 00:33:45, DaleCurtis wrote: > > > > the renderer_impl tests have mocking for GetStream() already. Should be > fairly > > > > trivial to setup this test there. It serves as a good unit test even if it > > > > doesn't test the exact bug you're trying to fix here. > > > > > > Wait, we seem to be talking about different things. I was thinking about > > > creating the test along the lines: > > > 1. Initialize WMPI and verify that Demuxer::GetStream returns a valid > stream. > > > 2. Emulate MediaSource/SourceBuffer being destroyed for WMPI and verify that > > > Demuxer::GetStream returns null after that. > > > > > > You are talking about renderer_impl test, but it's not clear what we would > test > > > there. After this change we will no longer call GetStream twice for each > stream > > > type, so what could we verify in there? Sure we could set up the GetStream > mock > > > to return the valid stream only the first time and return null on subsequent > > > invocations. But GetStream should only get invoked once per a/v renderer > init > > > after this change. I guess we could enforce that GetStream mock is only > invoked > > > once for each stream type during renderer init - is that what you meant? > > > > Btw, we already have checks that Demuxer::GetStream returns nulls after > ChunkDemuxer::RemoveId, see > https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer_unittest.cc?... > > Just having a test which fails on a second GetStream or returns null is valid > and recommended here. It'll help catch future races. It's kind of hard to do this because GetStream might get called variable number of times during RendererImpl::Initialize. For example even after this change GetStream(AUDIO) will get invoked 3 or 4 times: 1. https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=148... 2. https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=148... 3. through HasEncryptedStream at https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=148... 4. finally in the InitializeAudioRenderer at https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=148... And how many times the GetStream(VIDEO) will get invoked is going to depend on whether audio stream is present and encrypted or not. So while I guess we could calculate and set the exact number of times we expect GetStream to be invoked in RendererImpl test, I feel that this might be too fragile and prone to test breakage due to legitimate code changes later. Perhaps a better fix for this race would be for the HTMLMediaElement to wait for the pending pipeline initialization to finish completely before it starts shutting things down. But so far I can't find any hooks in the HTMLMediaElement that would indicate whether the pipeline/renderer init is complete or still going on, I feel that HTMLMediaElement must know this, but can't see anything obvious. Does WMPI let HTMLMediaElement know somehow when WebMediaPlayerImpl::load has finished completely, including all async operations, like renderer init?
https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:134: DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) || On 2016/12/08 23:01:02, wolenetz wrote: > It looks like this DCHECK might also fail if pipeline stops fast enough that we > lose the race here. Yes, I believe this is possible. And we also have the same race in RendererImpl::InitializeAudioRenderer, but I've checked out of curiousity and haven't found any crashes in RendererImpl::InitializeAudioRenderer so far, only in InitializeVideoRenderer. I believe that's because MediaSource destruction takes some time to remove media tracks and sent track notification events, etc, before notifying ChunkDemuxer about SourceBuffer removal. So video renderer init, being the last step of RendererImpl init has a higher chance of losing this race. https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:143: if (audio_stream) On 2016/12/08 23:01:02, wolenetz wrote: > Does it matter that we're setting up these CB's here on a stream that we might > not actually use (since GetStream in Initialize{Audio,Video}Renderer later might > return null)? It doesn't matter, since the race is only happening while the main thread is shutting down the pipeline while the media thread haven't finished initialization yet. So these demuxer streams and a/v renderers are going to be destroyed soon afterwards anyway.
https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:134: DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) || On 2016/12/09 00:26:31, servolk wrote: > On 2016/12/08 23:01:02, wolenetz wrote: > > It looks like this DCHECK might also fail if pipeline stops fast enough that > we > > lose the race here. > > Yes, I believe this is possible. And we also have the same race in > RendererImpl::InitializeAudioRenderer, but I've checked out of curiousity and > haven't found any crashes in RendererImpl::InitializeAudioRenderer so far, only > in InitializeVideoRenderer. I believe that's because MediaSource destruction > takes some time to remove media tracks and sent track notification events, etc, > before notifying ChunkDemuxer about SourceBuffer removal. So video renderer > init, being the last step of RendererImpl init has a higher chance of losing > this race. My point is DCHECKs are meant partially to document preconditions. If this DCHECK *can* fail here, we should either: 1) Change the DCHECK condition (change the precondition), or 2) Prevent this DCHECK from failing in the first place. Note that only debug builds (or Release built with dcheck_always_on) will even evaluate DCHECKs, so I wouldn't expect to see many/any crashes for something that already has a low Release-build crash rate. tl;dr: Is this DCHECK still ok? I think maybe we should short-circuit ::Initialize() here if the condition in this DCHECK would fail, and drop the DCHECK. WDYT? https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:143: if (audio_stream) On 2016/12/09 00:26:31, servolk wrote: > On 2016/12/08 23:01:02, wolenetz wrote: > > Does it matter that we're setting up these CB's here on a stream that we might > > not actually use (since GetStream in Initialize{Audio,Video}Renderer later > might > > return null)? > > It doesn't matter, since the race is only happening while the main thread is > shutting down the pipeline while the media thread haven't finished > initialization yet. So these demuxer streams and a/v renderers are going to be > destroyed soon afterwards anyway. Acknowledged.
https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:134: DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) || On 2016/12/09 23:23:00, wolenetz wrote: > On 2016/12/09 00:26:31, servolk wrote: > > On 2016/12/08 23:01:02, wolenetz wrote: > > > It looks like this DCHECK might also fail if pipeline stops fast enough that > > we > > > lose the race here. > > > > Yes, I believe this is possible. And we also have the same race in > > RendererImpl::InitializeAudioRenderer, but I've checked out of curiousity and > > haven't found any crashes in RendererImpl::InitializeAudioRenderer so far, > only > > in InitializeVideoRenderer. I believe that's because MediaSource destruction > > takes some time to remove media tracks and sent track notification events, > etc, > > before notifying ChunkDemuxer about SourceBuffer removal. So video renderer > > init, being the last step of RendererImpl init has a higher chance of losing > > this race. > > My point is DCHECKs are meant partially to document preconditions. If this > DCHECK *can* fail here, we should either: > 1) Change the DCHECK condition (change the precondition), or > 2) Prevent this DCHECK from failing in the first place. > > Note that only debug builds (or Release built with dcheck_always_on) will even > evaluate DCHECKs, so I wouldn't expect to see many/any crashes for something > that already has a low Release-build crash rate. > > tl;dr: Is this DCHECK still ok? I think maybe we should short-circuit > ::Initialize() here if the condition in this DCHECK would fail, and drop the > DCHECK. WDYT? After spending some more time thinking about this issue I think a better way to completely avoid this race would be to postpone MediaSource destruction until after WMPI/pipeline/renderer init is complete. If I can find a way to do that, then this DCHECK would stay.
On 2016/12/09 23:31:07, servolk wrote: > https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... > File media/renderers/renderer_impl.cc (right): > > https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... > media/renderers/renderer_impl.cc:134: > DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) || > On 2016/12/09 23:23:00, wolenetz wrote: > > On 2016/12/09 00:26:31, servolk wrote: > > > On 2016/12/08 23:01:02, wolenetz wrote: > > > > It looks like this DCHECK might also fail if pipeline stops fast enough > that > > > we > > > > lose the race here. > > > > > > Yes, I believe this is possible. And we also have the same race in > > > RendererImpl::InitializeAudioRenderer, but I've checked out of curiousity > and > > > haven't found any crashes in RendererImpl::InitializeAudioRenderer so far, > > only > > > in InitializeVideoRenderer. I believe that's because MediaSource destruction > > > takes some time to remove media tracks and sent track notification events, > > etc, > > > before notifying ChunkDemuxer about SourceBuffer removal. So video renderer > > > init, being the last step of RendererImpl init has a higher chance of losing > > > this race. > > > > My point is DCHECKs are meant partially to document preconditions. If this > > DCHECK *can* fail here, we should either: > > 1) Change the DCHECK condition (change the precondition), or > > 2) Prevent this DCHECK from failing in the first place. > > > > Note that only debug builds (or Release built with dcheck_always_on) will even > > evaluate DCHECKs, so I wouldn't expect to see many/any crashes for something > > that already has a low Release-build crash rate. > > > > tl;dr: Is this DCHECK still ok? I think maybe we should short-circuit > > ::Initialize() here if the condition in this DCHECK would fail, and drop the > > DCHECK. WDYT? > > After spending some more time thinking about this issue I think a better way to > completely avoid this race would be to postpone MediaSource destruction until > after WMPI/pipeline/renderer init is complete. If I can find a way to do that, > then this DCHECK would stay. Actually, I did some more investigation today and while it is possible to notify HTMLMediaElement about pipeline initialization completion (e.g. something like https://codereview.chromium.org/2564253002/), I've realized that if we went that route, we would have to block the main thread until the media thread finishes initializing pipeline and renderers. And blocking the main thread is bad of course. So I'm now planning to proceed with this initial solution. Since Javascript code might decide to reset HTMLMediaElement or remove SourceBuffer from the MediaSource at any time, we just need to be aware that DemuxerStreamProvider::GetStream might return NULL on the media thread even if the previous GetStream call returned non-NULL stream. On the upside at least we guarantee that the stream pointer previously returned by GetStream function is going to be valid for as long as ChunkDemuxer is alive, but reading from that stream will immediately return EOS. I've did a quick overview of other parts of code that use GetStream and I think we should be ok. I'm still not sure of a good way to add a test coverage for this though. As I've explained in https://codereview.chromium.org/2558213002/#msg20 we could try to set up expectations for how many times DemuxerStreamProvider::GetStream gets called during renderer initialization, but that's quite fragile and prone to breaking even with valid code changes. Do we still want to do that then?
On 2016/12/10 at 02:32:57, servolk wrote: > On 2016/12/09 23:31:07, servolk wrote: > > https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... > > File media/renderers/renderer_impl.cc (right): > > > > https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... > > media/renderers/renderer_impl.cc:134: > > DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) || > > On 2016/12/09 23:23:00, wolenetz wrote: > > > On 2016/12/09 00:26:31, servolk wrote: > > > > On 2016/12/08 23:01:02, wolenetz wrote: > > > > > It looks like this DCHECK might also fail if pipeline stops fast enough > > that > > > > we > > > > > lose the race here. > > > > > > > > Yes, I believe this is possible. And we also have the same race in > > > > RendererImpl::InitializeAudioRenderer, but I've checked out of curiousity > > and > > > > haven't found any crashes in RendererImpl::InitializeAudioRenderer so far, > > > only > > > > in InitializeVideoRenderer. I believe that's because MediaSource destruction > > > > takes some time to remove media tracks and sent track notification events, > > > etc, > > > > before notifying ChunkDemuxer about SourceBuffer removal. So video renderer > > > > init, being the last step of RendererImpl init has a higher chance of losing > > > > this race. > > > > > > My point is DCHECKs are meant partially to document preconditions. If this > > > DCHECK *can* fail here, we should either: > > > 1) Change the DCHECK condition (change the precondition), or > > > 2) Prevent this DCHECK from failing in the first place. > > > > > > Note that only debug builds (or Release built with dcheck_always_on) will even > > > evaluate DCHECKs, so I wouldn't expect to see many/any crashes for something > > > that already has a low Release-build crash rate. > > > > > > tl;dr: Is this DCHECK still ok? I think maybe we should short-circuit > > > ::Initialize() here if the condition in this DCHECK would fail, and drop the > > > DCHECK. WDYT? > > > > After spending some more time thinking about this issue I think a better way to > > completely avoid this race would be to postpone MediaSource destruction until > > after WMPI/pipeline/renderer init is complete. If I can find a way to do that, > > then this DCHECK would stay. > > Actually, I did some more investigation today and while it is possible to notify HTMLMediaElement about pipeline initialization completion (e.g. something like https://codereview.chromium.org/2564253002/), I've realized that if we went that route, we would have to block the main thread until the media thread finishes initializing pipeline and renderers. And blocking the main thread is bad of course. So I'm now planning to proceed with this initial solution. > Since Javascript code might decide to reset HTMLMediaElement or remove SourceBuffer from the MediaSource at any time, we just need to be aware that DemuxerStreamProvider::GetStream might return NULL on the media thread even if the previous GetStream call returned non-NULL stream. On the upside at least we guarantee that the stream pointer previously returned by GetStream function is going to be valid for as long as ChunkDemuxer is alive, but reading from that stream will immediately return EOS. I've did a quick overview of other parts of code that use GetStream and I think we should be ok. > I'm still not sure of a good way to add a test coverage for this though. As I've explained in https://codereview.chromium.org/2558213002/#msg20 we could try to set up expectations for how many times DemuxerStreamProvider::GetStream gets called during renderer initialization, but that's quite fragile and prone to breaking even with valid code changes. Do we still want to do that then? Why do you think that's fragile? Just writing a single test which checks this for Initialize() doesn't seem that fragile.
On 2016/12/12 22:17:20, DaleCurtis wrote: > On 2016/12/10 at 02:32:57, servolk wrote: > > On 2016/12/09 23:31:07, servolk wrote: > > > > https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... > > > File media/renderers/renderer_impl.cc (right): > > > > > > > https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... > > > media/renderers/renderer_impl.cc:134: > > > DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) || > > > On 2016/12/09 23:23:00, wolenetz wrote: > > > > On 2016/12/09 00:26:31, servolk wrote: > > > > > On 2016/12/08 23:01:02, wolenetz wrote: > > > > > > It looks like this DCHECK might also fail if pipeline stops fast > enough > > > that > > > > > we > > > > > > lose the race here. > > > > > > > > > > Yes, I believe this is possible. And we also have the same race in > > > > > RendererImpl::InitializeAudioRenderer, but I've checked out of > curiousity > > > and > > > > > haven't found any crashes in RendererImpl::InitializeAudioRenderer so > far, > > > > only > > > > > in InitializeVideoRenderer. I believe that's because MediaSource > destruction > > > > > takes some time to remove media tracks and sent track notification > events, > > > > etc, > > > > > before notifying ChunkDemuxer about SourceBuffer removal. So video > renderer > > > > > init, being the last step of RendererImpl init has a higher chance of > losing > > > > > this race. > > > > > > > > My point is DCHECKs are meant partially to document preconditions. If this > > > > DCHECK *can* fail here, we should either: > > > > 1) Change the DCHECK condition (change the precondition), or > > > > 2) Prevent this DCHECK from failing in the first place. > > > > > > > > Note that only debug builds (or Release built with dcheck_always_on) will > even > > > > evaluate DCHECKs, so I wouldn't expect to see many/any crashes for > something > > > > that already has a low Release-build crash rate. > > > > > > > > tl;dr: Is this DCHECK still ok? I think maybe we should short-circuit > > > > ::Initialize() here if the condition in this DCHECK would fail, and drop > the > > > > DCHECK. WDYT? > > > > > > After spending some more time thinking about this issue I think a better way > to > > > completely avoid this race would be to postpone MediaSource destruction > until > > > after WMPI/pipeline/renderer init is complete. If I can find a way to do > that, > > > then this DCHECK would stay. > > > > Actually, I did some more investigation today and while it is possible to > notify HTMLMediaElement about pipeline initialization completion (e.g. something > like https://codereview.chromium.org/2564253002/), I've realized that if we went > that route, we would have to block the main thread until the media thread > finishes initializing pipeline and renderers. And blocking the main thread is > bad of course. So I'm now planning to proceed with this initial solution. > > Since Javascript code might decide to reset HTMLMediaElement or remove > SourceBuffer from the MediaSource at any time, we just need to be aware that > DemuxerStreamProvider::GetStream might return NULL on the media thread even if > the previous GetStream call returned non-NULL stream. On the upside at least we > guarantee that the stream pointer previously returned by GetStream function is > going to be valid for as long as ChunkDemuxer is alive, but reading from that > stream will immediately return EOS. I've did a quick overview of other parts of > code that use GetStream and I think we should be ok. > > I'm still not sure of a good way to add a test coverage for this though. As > I've explained in https://codereview.chromium.org/2558213002/#msg20 we could try > to set up expectations for how many times DemuxerStreamProvider::GetStream gets > called during renderer initialization, but that's quite fragile and prone to > breaking even with valid code changes. Do we still want to do that then? > > Why do you think that's fragile? Just writing a single test which checks this > for Initialize() doesn't seem that fragile. It's fragile because somebody might call DemuxerStreamProvider::GetStream one more time during the renderer initialization (and it would be totally fine, as long as the result of GetStream is checked to be non-null before being used) and that would break the test, since the number of invocations of GetStream has changed. Hopefully that won't be too often, but it's hard to tell and it's really easy to add more accidental calls of GetStream e.g. by calling HasEncryptedStream() one more time.
On 2016/12/12 at 22:24:26, servolk wrote: > On 2016/12/12 22:17:20, DaleCurtis wrote: > > On 2016/12/10 at 02:32:57, servolk wrote: > > > On 2016/12/09 23:31:07, servolk wrote: > > > > > > https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... > > > > File media/renderers/renderer_impl.cc (right): > > > > > > > > > > https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... > > > > media/renderers/renderer_impl.cc:134: > > > > DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) || > > > > On 2016/12/09 23:23:00, wolenetz wrote: > > > > > On 2016/12/09 00:26:31, servolk wrote: > > > > > > On 2016/12/08 23:01:02, wolenetz wrote: > > > > > > > It looks like this DCHECK might also fail if pipeline stops fast > > enough > > > > that > > > > > > we > > > > > > > lose the race here. > > > > > > > > > > > > Yes, I believe this is possible. And we also have the same race in > > > > > > RendererImpl::InitializeAudioRenderer, but I've checked out of > > curiousity > > > > and > > > > > > haven't found any crashes in RendererImpl::InitializeAudioRenderer so > > far, > > > > > only > > > > > > in InitializeVideoRenderer. I believe that's because MediaSource > > destruction > > > > > > takes some time to remove media tracks and sent track notification > > events, > > > > > etc, > > > > > > before notifying ChunkDemuxer about SourceBuffer removal. So video > > renderer > > > > > > init, being the last step of RendererImpl init has a higher chance of > > losing > > > > > > this race. > > > > > > > > > > My point is DCHECKs are meant partially to document preconditions. If this > > > > > DCHECK *can* fail here, we should either: > > > > > 1) Change the DCHECK condition (change the precondition), or > > > > > 2) Prevent this DCHECK from failing in the first place. > > > > > > > > > > Note that only debug builds (or Release built with dcheck_always_on) will > > even > > > > > evaluate DCHECKs, so I wouldn't expect to see many/any crashes for > > something > > > > > that already has a low Release-build crash rate. > > > > > > > > > > tl;dr: Is this DCHECK still ok? I think maybe we should short-circuit > > > > > ::Initialize() here if the condition in this DCHECK would fail, and drop > > the > > > > > DCHECK. WDYT? > > > > > > > > After spending some more time thinking about this issue I think a better way > > to > > > > completely avoid this race would be to postpone MediaSource destruction > > until > > > > after WMPI/pipeline/renderer init is complete. If I can find a way to do > > that, > > > > then this DCHECK would stay. > > > > > > Actually, I did some more investigation today and while it is possible to > > notify HTMLMediaElement about pipeline initialization completion (e.g. something > > like https://codereview.chromium.org/2564253002/), I've realized that if we went > > that route, we would have to block the main thread until the media thread > > finishes initializing pipeline and renderers. And blocking the main thread is > > bad of course. So I'm now planning to proceed with this initial solution. > > > Since Javascript code might decide to reset HTMLMediaElement or remove > > SourceBuffer from the MediaSource at any time, we just need to be aware that > > DemuxerStreamProvider::GetStream might return NULL on the media thread even if > > the previous GetStream call returned non-NULL stream. On the upside at least we > > guarantee that the stream pointer previously returned by GetStream function is > > going to be valid for as long as ChunkDemuxer is alive, but reading from that > > stream will immediately return EOS. I've did a quick overview of other parts of > > code that use GetStream and I think we should be ok. > > > I'm still not sure of a good way to add a test coverage for this though. As > > I've explained in https://codereview.chromium.org/2558213002/#msg20 we could try > > to set up expectations for how many times DemuxerStreamProvider::GetStream gets > > called during renderer initialization, but that's quite fragile and prone to > > breaking even with valid code changes. Do we still want to do that then? > > > > Why do you think that's fragile? Just writing a single test which checks this > > for Initialize() doesn't seem that fragile. > > It's fragile because somebody might call DemuxerStreamProvider::GetStream one more time during the renderer initialization (and it would be totally fine, as long as the result of GetStream is checked to be non-null before being used) and that would break the test, since the number of invocations of GetStream has changed. Hopefully that won't be too often, but it's hard to tell and it's really easy to add more accidental calls of GetStream e.g. by calling HasEncryptedStream() one more time. I think that's a valid non-fragile test. It's surprising that GetStream() results may change during the lifetime of a single function and having a test which enforces this to some extent is usefuol.
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 2016/12/12 23:04:02, DaleCurtis wrote: > On 2016/12/12 at 22:24:26, servolk wrote: > > On 2016/12/12 22:17:20, DaleCurtis wrote: > > > On 2016/12/10 at 02:32:57, servolk wrote: > > > > On 2016/12/09 23:31:07, servolk wrote: > > > > > > > > > https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... > > > > > File media/renderers/renderer_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2558213002/diff/1/media/renderers/renderer_im... > > > > > media/renderers/renderer_impl.cc:134: > > > > > DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) || > > > > > On 2016/12/09 23:23:00, wolenetz wrote: > > > > > > On 2016/12/09 00:26:31, servolk wrote: > > > > > > > On 2016/12/08 23:01:02, wolenetz wrote: > > > > > > > > It looks like this DCHECK might also fail if pipeline stops fast > > > enough > > > > > that > > > > > > > we > > > > > > > > lose the race here. > > > > > > > > > > > > > > Yes, I believe this is possible. And we also have the same race in > > > > > > > RendererImpl::InitializeAudioRenderer, but I've checked out of > > > curiousity > > > > > and > > > > > > > haven't found any crashes in RendererImpl::InitializeAudioRenderer > so > > > far, > > > > > > only > > > > > > > in InitializeVideoRenderer. I believe that's because MediaSource > > > destruction > > > > > > > takes some time to remove media tracks and sent track notification > > > events, > > > > > > etc, > > > > > > > before notifying ChunkDemuxer about SourceBuffer removal. So video > > > renderer > > > > > > > init, being the last step of RendererImpl init has a higher chance > of > > > losing > > > > > > > this race. > > > > > > > > > > > > My point is DCHECKs are meant partially to document preconditions. If > this > > > > > > DCHECK *can* fail here, we should either: > > > > > > 1) Change the DCHECK condition (change the precondition), or > > > > > > 2) Prevent this DCHECK from failing in the first place. > > > > > > > > > > > > Note that only debug builds (or Release built with dcheck_always_on) > will > > > even > > > > > > evaluate DCHECKs, so I wouldn't expect to see many/any crashes for > > > something > > > > > > that already has a low Release-build crash rate. > > > > > > > > > > > > tl;dr: Is this DCHECK still ok? I think maybe we should short-circuit > > > > > > ::Initialize() here if the condition in this DCHECK would fail, and > drop > > > the > > > > > > DCHECK. WDYT? > > > > > > > > > > After spending some more time thinking about this issue I think a better > way > > > to > > > > > completely avoid this race would be to postpone MediaSource destruction > > > until > > > > > after WMPI/pipeline/renderer init is complete. If I can find a way to do > > > that, > > > > > then this DCHECK would stay. > > > > > > > > Actually, I did some more investigation today and while it is possible to > > > notify HTMLMediaElement about pipeline initialization completion (e.g. > something > > > like https://codereview.chromium.org/2564253002/), I've realized that if we > went > > > that route, we would have to block the main thread until the media thread > > > finishes initializing pipeline and renderers. And blocking the main thread > is > > > bad of course. So I'm now planning to proceed with this initial solution. > > > > Since Javascript code might decide to reset HTMLMediaElement or remove > > > SourceBuffer from the MediaSource at any time, we just need to be aware that > > > DemuxerStreamProvider::GetStream might return NULL on the media thread even > if > > > the previous GetStream call returned non-NULL stream. On the upside at least > we > > > guarantee that the stream pointer previously returned by GetStream function > is > > > going to be valid for as long as ChunkDemuxer is alive, but reading from > that > > > stream will immediately return EOS. I've did a quick overview of other parts > of > > > code that use GetStream and I think we should be ok. > > > > I'm still not sure of a good way to add a test coverage for this though. > As > > > I've explained in https://codereview.chromium.org/2558213002/#msg20 we could > try > > > to set up expectations for how many times DemuxerStreamProvider::GetStream > gets > > > called during renderer initialization, but that's quite fragile and prone to > > > breaking even with valid code changes. Do we still want to do that then? > > > > > > Why do you think that's fragile? Just writing a single test which checks > this > > > for Initialize() doesn't seem that fragile. > > > > It's fragile because somebody might call DemuxerStreamProvider::GetStream one > more time during the renderer initialization (and it would be totally fine, as > long as the result of GetStream is checked to be non-null before being used) and > that would break the test, since the number of invocations of GetStream has > changed. Hopefully that won't be too often, but it's hard to tell and it's > really easy to add more accidental calls of GetStream e.g. by calling > HasEncryptedStream() one more time. > > I think that's a valid non-fragile test. It's surprising that GetStream() > results may change during the lifetime of a single function and having a test > which enforces this to some extent is usefuol. Ok, I've added more stricter DemuxerStreamProvider::GetStream expectations in RendererImpl tests + some comments explaining why those might fail, PTAL at latest patchset. I've also removed DCHECK at https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=0&l..., since as Matt pointer out it might get triggered due to the same race (or should we replace it with if (!audio_stream && !video_stream) { return; }, i.e. abandon renderer init if there's no streams?).
dalecurtis@chromium.org changed reviewers: + xhwang@chromium.org
https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:139: DemuxerStream* audio_stream = Can you move these into InitializeAudioRenderer() and InitializeVideoRenderer() respectively? https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:141: if (audio_stream) Needs {} https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:146: if (video_stream) Multiline if needs {} https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:376: void RendererImpl::InitializeAudioRenderer() { Pass in DemuxerStream* here and add a comment about why we do so if you can't move the setters. https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl_unittest.cc:161: EXPECT_CALL(*demuxer_, GetStream(DemuxerStream::AUDIO)) For HasEncryptedStream() can we cache the value during InitializeAudioRenderer() / InitializeVideoRenderer() or does this change later? +xhwang for clarity.
https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:139: DemuxerStream* audio_stream = On 2016/12/12 23:43:09, DaleCurtis wrote: > Can you move these into InitializeAudioRenderer() and InitializeVideoRenderer() > respectively? Yes, done. https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:141: if (audio_stream) On 2016/12/12 23:43:09, DaleCurtis wrote: > Needs {} The if is no longer necessary now that it's called from InitializeAudioRenderer https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:146: if (video_stream) On 2016/12/12 23:43:09, DaleCurtis wrote: > Multiline if needs {} ditto https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:376: void RendererImpl::InitializeAudioRenderer() { On 2016/12/12 23:43:09, DaleCurtis wrote: > Pass in DemuxerStream* here and add a comment about why we do so if you can't > move the setters. I guess we could pass DemuxerStream* in here, but InitializeAudioRenderer might get called either from RendererImpl::Initialize directly or from SetCdm if we need to init CDM first. If we want to be strict about always calling GetStream once per each stream type I guess we could call those once in RendererImpl::Initialize and then always reuse the cached values. Or we could just call GetStream once inside RendererImpl::InitializeAudio/VideoRenderer and allow HasEncryptedStream to call GetStream one more time, it's probably not a big deal.
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 2016/12/12 at 23:59:23, servolk wrote: > https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... > File media/renderers/renderer_impl.cc (right): > > https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... > media/renderers/renderer_impl.cc:139: DemuxerStream* audio_stream = > On 2016/12/12 23:43:09, DaleCurtis wrote: > > Can you move these into InitializeAudioRenderer() and InitializeVideoRenderer() > > respectively? > > Yes, done. > > https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... > media/renderers/renderer_impl.cc:141: if (audio_stream) > On 2016/12/12 23:43:09, DaleCurtis wrote: > > Needs {} > > The if is no longer necessary now that it's called from InitializeAudioRenderer > > https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... > media/renderers/renderer_impl.cc:146: if (video_stream) > On 2016/12/12 23:43:09, DaleCurtis wrote: > > Multiline if needs {} > > ditto > > https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... > media/renderers/renderer_impl.cc:376: void RendererImpl::InitializeAudioRenderer() { > On 2016/12/12 23:43:09, DaleCurtis wrote: > > Pass in DemuxerStream* here and add a comment about why we do so if you can't > > move the setters. > > I guess we could pass DemuxerStream* in here, but InitializeAudioRenderer might get called either from RendererImpl::Initialize directly or from SetCdm if we need to init CDM first. If we want to be strict about always calling GetStream once per each stream type I guess we could call those once in RendererImpl::Initialize and then always reuse the cached values. Or we could just call GetStream once inside RendererImpl::InitializeAudio/VideoRenderer and allow HasEncryptedStream to call GetStream one more time, it's probably not a big deal. We need to be careful about using cached values. If we cache anything it shoudln't be a stream pointer, but instead just whatever value we're looking for (is_encrypted in the case above). The return value is only valid within a single cycle of the media message loop. Which is pretty hairy in and of itself I think.
https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl_unittest.cc:161: EXPECT_CALL(*demuxer_, GetStream(DemuxerStream::AUDIO)) On 2016/12/12 23:43:09, DaleCurtis wrote: > For HasEncryptedStream() can we cache the value during InitializeAudioRenderer() > / InitializeVideoRenderer() or does this change later? +xhwang for clarity. Not having the big picture of this CL... :) Today HasEncryptedStream() is called before InitializeAudioRenderer()/InitializeVideoRenderer(), and the value really depends on whether we have any encrypted DemuxerStream. But it's a little bit vague to me when the streams can change. I assume the streams cannot change when we are pending Renderer::Initialize()? Then after the initialization succeeded and we are playing, some streams can be disabled in the form of EOS. Is this correct? If we disable the encrypted stream, I assume HasEncryptedStream() will become false, but I suppose this should not happen during Renderer::Initialize()? FWIW, today RendererImpl::Initialize() will wait for a CDM to be set to start initialize audio/video renderers if HasEncryptedStream() is true. I am thinking that we probably should move this logic to DecoderSelector so that we can handle more dynamic config changes, e.g. switch between clear and encrytped. Will that do any help here?
On 2016/12/13 00:07:29, xhwang wrote: > https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... > File media/renderers/renderer_impl_unittest.cc (right): > > https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... > media/renderers/renderer_impl_unittest.cc:161: EXPECT_CALL(*demuxer_, > GetStream(DemuxerStream::AUDIO)) > On 2016/12/12 23:43:09, DaleCurtis wrote: > > For HasEncryptedStream() can we cache the value during > InitializeAudioRenderer() > > / InitializeVideoRenderer() or does this change later? +xhwang for clarity. > > Not having the big picture of this CL... :) > > Today HasEncryptedStream() is called before > InitializeAudioRenderer()/InitializeVideoRenderer(), and the value really > depends on whether we have any encrypted DemuxerStream. > > But it's a little bit vague to me when the streams can change. > > I assume the streams cannot change when we are pending Renderer::Initialize()? > Then after the initialization succeeded and we are playing, some streams can be > disabled in the form of EOS. Is this correct? If we disable the encrypted > stream, I assume HasEncryptedStream() will become false, but I suppose this > should not happen during Renderer::Initialize()? > > FWIW, today RendererImpl::Initialize() will wait for a CDM to be set to start > initialize audio/video renderers if HasEncryptedStream() is true. I am thinking > that we probably should move this logic to DecoderSelector so that we can handle > more dynamic config changes, e.g. switch between clear and encrytped. Will that > do any help here? Xiaohan, please see https://bugs.chromium.org/p/chromium/issues/detail?id=668604#c11 for a bit more context. To summarize it here: the return value of DemuxerStreamProvider::GetStream might become null during the course of RendererImpl::Initialize even if previously GetStream returned non-null for the same stream type. This happens when HTMLMediaElement is destroying blink MediaSource on the main thread while the RendererImpl is being initialized on the media thread. Non-null streams returned from DemuxerStreamProvider::GetStream earlier are guaranteed to stay valid and can be cached safely (since ChunkDemuxer::RemoveId makes streams inaccessible, but keeps them alive). I think we should be fine here, since HasEncryptedStream only calls GetStream once per each stream type and checks the result being non-null before using it. The problem that this CL fixes is that RendererImpl::InitializeAudio/VideoRenderer are currently calling GetStream twice and assume that if the first invocation returned non-null pointer then the second invocation is also going to return non-null.
On 2016/12/13 00:07:25, DaleCurtis wrote: > On 2016/12/12 at 23:59:23, servolk wrote: > > > https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... > > File media/renderers/renderer_impl.cc (right): > > > > > https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... > > media/renderers/renderer_impl.cc:139: DemuxerStream* audio_stream = > > On 2016/12/12 23:43:09, DaleCurtis wrote: > > > Can you move these into InitializeAudioRenderer() and > InitializeVideoRenderer() > > > respectively? > > > > Yes, done. > > > > > https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... > > media/renderers/renderer_impl.cc:141: if (audio_stream) > > On 2016/12/12 23:43:09, DaleCurtis wrote: > > > Needs {} > > > > The if is no longer necessary now that it's called from > InitializeAudioRenderer > > > > > https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... > > media/renderers/renderer_impl.cc:146: if (video_stream) > > On 2016/12/12 23:43:09, DaleCurtis wrote: > > > Multiline if needs {} > > > > ditto > > > > > https://codereview.chromium.org/2558213002/diff/20001/media/renderers/rendere... > > media/renderers/renderer_impl.cc:376: void > RendererImpl::InitializeAudioRenderer() { > > On 2016/12/12 23:43:09, DaleCurtis wrote: > > > Pass in DemuxerStream* here and add a comment about why we do so if you > can't > > > move the setters. > > > > I guess we could pass DemuxerStream* in here, but InitializeAudioRenderer > might get called either from RendererImpl::Initialize directly or from SetCdm if > we need to init CDM first. If we want to be strict about always calling > GetStream once per each stream type I guess we could call those once in > RendererImpl::Initialize and then always reuse the cached values. Or we could > just call GetStream once inside RendererImpl::InitializeAudio/VideoRenderer and > allow HasEncryptedStream to call GetStream one more time, it's probably not a > big deal. > > We need to be careful about using cached values. If we cache anything it > shoudln't be a stream pointer, but instead just whatever value we're looking for > (is_encrypted in the case above). The return value is only valid within a single > cycle of the media message loop. Which is pretty hairy in and of itself I think. I think we are actually fine even with caching the stream pointer, since the stream is going to be kept alive. ChunkDemuxer::RemoveId makes streams inaccessible, but keeps them alive in ChunkDemuxer::removed_streams_ !
OIC, I thought it was destroying them at some later date. I see now that it keeps them alive until demuxer destruction. Ensuring HasEncryptedStream() doesn't waffle still sgtm, but this lgtm as is. https://codereview.chromium.org/2558213002/diff/40001/media/renderers/rendere... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2558213002/diff/40001/media/renderers/rendere... media/renderers/renderer_impl.cc:383: &RendererImpl::RestartStreamPlayback, weak_this_, audio_stream)); Should we be binding in the audio_stream pointer like this? If the stream goes away, what happens if this callback is called later. Seems like demuxer_stream_provider should be passed in here? https://codereview.chromium.org/2558213002/diff/40001/media/renderers/rendere... media/renderers/renderer_impl.cc:432: &RendererImpl::RestartStreamPlayback, weak_this_, video_stream)); Ditto.
I might still be missing some context, but... Can we somehow fix our API such that at least during Renderer::Initialize() the streams returned by GetStream() do not change? This seems really scary to me... If we initialized on a "removed stream", what will happen when we call Read() on that stream? We can fix RendererImpl here, but how about other Renderers? For example, does MojoRenderer + CastRenderer work? If we decide to keep this, can we update our documentation to make it clear that streams can change? e.g. on Renderer::Initialize() and/or DemuxerStreamProvider::GetStream().
On 2016/12/13 00:31:38, DaleCurtis wrote: > OIC, I thought it was destroying them at some later date. I see now that it > keeps them alive until demuxer destruction. Ensuring HasEncryptedStream() > doesn't waffle still sgtm, but this lgtm as is. > > https://codereview.chromium.org/2558213002/diff/40001/media/renderers/rendere... > File media/renderers/renderer_impl.cc (right): > > https://codereview.chromium.org/2558213002/diff/40001/media/renderers/rendere... > media/renderers/renderer_impl.cc:383: &RendererImpl::RestartStreamPlayback, > weak_this_, audio_stream)); > Should we be binding in the audio_stream pointer like this? If the stream goes > away, what happens if this callback is called later. Seems like > demuxer_stream_provider should be passed in here? > > https://codereview.chromium.org/2558213002/diff/40001/media/renderers/rendere... > media/renderers/renderer_impl.cc:432: &RendererImpl::RestartStreamPlayback, > weak_this_, video_stream)); > Ditto. Yeah, sorry, I should have emphasized this more. Once a pointer is returned from DemuxerStreamProvider::GetStream it's guaranteed to stay valid for as long as demuxer is alive. ChunkDemuxer keeps removed streams in ChunkDemuxer::removed_stream_ collection to ensure this, instead of destroying them right away. WMPI guarantees that Demuxer will outlive media::Pipeline and thus renderer. So renderers can safely make the assumption that streams returned by DemuxerStreamProvider::GetStream will stay valid.
https://codereview.chromium.org/2558213002/diff/40001/media/renderers/rendere... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2558213002/diff/40001/media/renderers/rendere... media/renderers/renderer_impl.cc:383: &RendererImpl::RestartStreamPlayback, weak_this_, audio_stream)); On 2016/12/13 00:31:38, DaleCurtis wrote: > Should we be binding in the audio_stream pointer like this? If the stream goes > away, what happens if this callback is called later. Seems like > demuxer_stream_provider should be passed in here? As I've explained in my previous reply, this should be fine. WMPI guarantees that Demuxer will outline Renderer, and any stream returned by DemuxerStreamProvider::GetStream should have the same lifetime as the Demuxer itself. https://codereview.chromium.org/2558213002/diff/40001/media/renderers/rendere... media/renderers/renderer_impl.cc:432: &RendererImpl::RestartStreamPlayback, weak_this_, video_stream)); On 2016/12/13 00:31:38, DaleCurtis wrote: > Ditto. Same.
On 2016/12/13 00:41:08, xhwang wrote: > I might still be missing some context, but... > > Can we somehow fix our API such that at least during Renderer::Initialize() the > streams returned by GetStream() do not change? This seems really scary to me... > > If we initialized on a "removed stream", what will happen when we call Read() on > that stream? > > We can fix RendererImpl here, but how about other Renderers? For example, does > MojoRenderer + CastRenderer work? > > If we decide to keep this, can we update our documentation to make it clear that > streams can change? e.g. on Renderer::Initialize() and/or > DemuxerStreamProvider::GetStream(). Xiaohan, please read what I just posted to Dale. Once a stream pointer is returned from DemuxerStreamProvider::GetStream it's guaranteed to stay valid for as long as the demuxer is alive. Those streams are in a well-defined SHUTDOWN state and reading from them will simply return EOS immediately, we have a test case for that, see https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer_unittest.cc?.... I don't think we can easily fix the API such that GetStream result doesn't change during Renderer::Initialize, since then we would need to block main UI/JS thread while the renderer is being initialized. I think it's a good idea to mention this in DemuxerStreamProvider::GetStream documentation, I'll add a comment.
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 2016/12/13 00:51:35, servolk wrote: > On 2016/12/13 00:41:08, xhwang wrote: > > I might still be missing some context, but... > > > > Can we somehow fix our API such that at least during Renderer::Initialize() > the > > streams returned by GetStream() do not change? This seems really scary to > me... > > > > If we initialized on a "removed stream", what will happen when we call Read() > on > > that stream? > > > > We can fix RendererImpl here, but how about other Renderers? For example, does > > MojoRenderer + CastRenderer work? > > > > If we decide to keep this, can we update our documentation to make it clear > that > > streams can change? e.g. on Renderer::Initialize() and/or > > DemuxerStreamProvider::GetStream(). > > Xiaohan, please read what I just posted to Dale. Once a stream pointer is > returned from DemuxerStreamProvider::GetStream it's guaranteed to stay valid for > as long as the demuxer is alive. > Those streams are in a well-defined SHUTDOWN state and reading from them will > simply return EOS immediately, we have a test case for that, see > https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer_unittest.cc?.... > I don't think we can easily fix the API such that GetStream result doesn't > change during Renderer::Initialize, since then we would need to block main UI/JS > thread while the renderer is being initialized. > I think it's a good idea to mention this in DemuxerStreamProvider::GetStream > documentation, I'll add a comment. Added a comment/explanation to DemuxerStreamProvider::GetStream in the latest patchset.
Thanks for the explanation and the new comments. This lg to me. However, heavy commenting usually is a sign of bad API design :( I wish we could have some better way to improve it. It should not block your CL though.
The CQ bit was unchecked by servolk@chromium.org
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/2558213002/#ps60001 (title: "Added a comment for DemuxerStreamProvider::GetStream")
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": 60001, "attempt_start_ts": 1481595219193090,
"parent_rev": "44cd835220fdf11e741195c549934e3b4eaf5a3a", "commit_rev":
"65d75e503e86405121438f7b3e3a1678b5590b60"}
Message was sent while issue was closed.
Description was changed from ========== Fix crash in renderer initialization when media pipeline is stopped The crash happens because by the time we call Demuxer::GetStream again the main thread might have proceeded with destroying SourceBuffer and thus ChunkDemuxer no longer allows accessing demuxer streams. Luckily demuxer streams themselves are still alive, just moved to ChunkDemuxer::removed_streams_ (see ChunkDemuxer::RemoveId). So cached pointers can still be used safely. BUG=668604 ========== to ========== Fix crash in renderer initialization when media pipeline is stopped The crash happens because by the time we call Demuxer::GetStream again the main thread might have proceeded with destroying SourceBuffer and thus ChunkDemuxer no longer allows accessing demuxer streams. Luckily demuxer streams themselves are still alive, just moved to ChunkDemuxer::removed_streams_ (see ChunkDemuxer::RemoveId). So cached pointers can still be used safely. BUG=668604 Review-Url: https://codereview.chromium.org/2558213002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix crash in renderer initialization when media pipeline is stopped The crash happens because by the time we call Demuxer::GetStream again the main thread might have proceeded with destroying SourceBuffer and thus ChunkDemuxer no longer allows accessing demuxer streams. Luckily demuxer streams themselves are still alive, just moved to ChunkDemuxer::removed_streams_ (see ChunkDemuxer::RemoveId). So cached pointers can still be used safely. BUG=668604 Review-Url: https://codereview.chromium.org/2558213002 ========== to ========== Fix crash in renderer initialization when media pipeline is stopped The crash happens because by the time we call Demuxer::GetStream again the main thread might have proceeded with destroying SourceBuffer and thus ChunkDemuxer no longer allows accessing demuxer streams. Luckily demuxer streams themselves are still alive, just moved to ChunkDemuxer::removed_streams_ (see ChunkDemuxer::RemoveId). So cached pointers can still be used safely. BUG=668604 Committed: https://crrev.com/ee02c5e1edfa2bfb94d115aa985db582bcf20d2e Cr-Commit-Position: refs/heads/master@{#438000} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ee02c5e1edfa2bfb94d115aa985db582bcf20d2e Cr-Commit-Position: refs/heads/master@{#438000} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
