Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
869283003:
Chromecast buildfix: update CmaRenderer for Initialize API change. (Closed)
|
Created:
5 years, 11 months ago by gunsch Modified:
5 years, 11 months ago CC:
chromium-reviews, gunsch+watch_chromium.org, lcwu+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromecast buildfix: update CmaRenderer for Initialize API change.
See: https://codereview.chromium.org/870693002
R=erickung@chromium.org,dalecurtis@chromium.org
BUG=450764
Committed: https://crrev.com/9ca5b72d0c04e2549d6374d86ef9ec7ac324010d
Cr-Commit-Position: refs/heads/master@{#313195}
Patch Set 1 #
Total comments: 8
Patch Set 2 : address dalecurtis comments #
Total comments: 6
Patch Set 3 : more aggressively exit in OnError #Patch Set 4 : else if flush_cb_ #
Total comments: 2
Patch Set 5 : address eric's style suggestion #Patch Set 6 : fixed error handling during initialization #Messages
Total messages: 17 (1 generated)
I'm not familiar with this code, but I think you're missing some checks. https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... File chromecast/media/cma/filters/cma_renderer.cc (right): https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... chromecast/media/cma/filters/cma_renderer.cc:329: Initialize may still complete (successfully or not) after an OnError event occurs. So you need to check if init_cb_ is null / state_ != error like I do in my change. https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... chromecast/media/cma/filters/cma_renderer.cc:434: error_cb_.Run(error); This shouldn't be run if init_cb_ is present.
Thanks for the review --- unfortunately getting a Chromecast trybot has been slow progress so this ends up an after-the-fact buildfix instead of earlier, but so it goes. The brief context is that this is Chromecast's ::media::Renderer, and it basically just proxies everything to the browser side for hardware-accelerated video rendering. https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... File chromecast/media/cma/filters/cma_renderer.cc (right): https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... chromecast/media/cma/filters/cma_renderer.cc:329: On 2015/01/24 01:01:56, DaleCurtis wrote: > Initialize may still complete (successfully or not) after an OnError event > occurs. So you need to check if init_cb_ is null / state_ != error like I do in > my change. Hmm, not sure I follow --- if an OnError occurs during initialization, that resets init_cb_ (as in your CL), in which case it seems to me that initialization won't be able to finish. I did notice I missed an OnError call in OnAudioPipelineInitializeDone, which corresponds to one in your change as well. https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... chromecast/media/cma/filters/cma_renderer.cc:434: error_cb_.Run(error); On 2015/01/24 01:01:56, DaleCurtis wrote: > This shouldn't be run if init_cb_ is present. Done.
https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... File chromecast/media/cma/filters/cma_renderer.cc (right): https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... chromecast/media/cma/filters/cma_renderer.cc:329: On 2015/01/24 01:20:19, gunsch wrote: > On 2015/01/24 01:01:56, DaleCurtis wrote: > > Initialize may still complete (successfully or not) after an OnError event > > occurs. So you need to check if init_cb_ is null / state_ != error like I do > in > > my change. > > Hmm, not sure I follow --- if an OnError occurs during initialization, that > resets init_cb_ (as in your CL), in which case it seems to me that > initialization won't be able to finish. > > I did notice I missed an OnError call in OnAudioPipelineInitializeDone, which > corresponds to one in your change as well. I don't know what your downstream looks like, but an OnError doesn't preclude a downstream initialize callback from firing. I.e. On desktop, the audio renderer may receive a device error at any time.
https://codereview.chromium.org/869283003/diff/20001/chromecast/media/cma/fil... File chromecast/media/cma/filters/cma_renderer.cc (right): https://codereview.chromium.org/869283003/diff/20001/chromecast/media/cma/fil... chromecast/media/cma/filters/cma_renderer.cc:67: if (!flush_cb_.is_null()) Should be an else here, https://codereview.chromium.org/869283003/diff/20001/chromecast/media/cma/fil... chromecast/media/cma/filters/cma_renderer.cc:429: bool isInitializing = !init_cb_.is_null(); is_initializing https://codereview.chromium.org/869283003/diff/20001/chromecast/media/cma/fil... chromecast/media/cma/filters/cma_renderer.cc:431: base::ResetAndReturn(&init_cb_).Run(error); This may immediately destroy your class, so you can't do anything else afterward.
https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... File chromecast/media/cma/filters/cma_renderer.cc (right): https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... chromecast/media/cma/filters/cma_renderer.cc:329: On 2015/01/24 01:24:06, DaleCurtis wrote: > On 2015/01/24 01:20:19, gunsch wrote: > > On 2015/01/24 01:01:56, DaleCurtis wrote: > > > Initialize may still complete (successfully or not) after an OnError event > > > occurs. So you need to check if init_cb_ is null / state_ != error like I do > > in > > > my change. > > > > Hmm, not sure I follow --- if an OnError occurs during initialization, that > > resets init_cb_ (as in your CL), in which case it seems to me that > > initialization won't be able to finish. > > > > I did notice I missed an OnError call in OnAudioPipelineInitializeDone, which > > corresponds to one in your change as well. > > I don't know what your downstream looks like, but an OnError doesn't preclude a > downstream initialize callback from firing. I.e. On desktop, the audio renderer > may receive a device error at any time. Still not following. Are you describing either of the following sequences, and if so can you clarify what I'm misunderstanding? Sequence 1 * OnError fired _before_ Initialize is called * Initialize won't be called, since Pipeline destroys Renderer Sequence 2 * Initialize called, init_cb_ set * A pipeline error occurs, OnError called * OnError fires/resets init_cb_ with the error code, since state is not already kError * Initialize won't finish, since Pipeline destroys Renderer From my reading of Pipeline, both error_cb_ and init_cb_(error status) look like they will destroy the Renderer instance. I've added the "state != error" check to OnError when calling init_cb_, but I guess I'm still not seeing a sequence in which that would actually be necessary. https://codereview.chromium.org/869283003/diff/20001/chromecast/media/cma/fil... File chromecast/media/cma/filters/cma_renderer.cc (right): https://codereview.chromium.org/869283003/diff/20001/chromecast/media/cma/fil... chromecast/media/cma/filters/cma_renderer.cc:429: bool isInitializing = !init_cb_.is_null(); On 2015/01/24 01:29:02, DaleCurtis wrote: > is_initializing Done. https://codereview.chromium.org/869283003/diff/20001/chromecast/media/cma/fil... chromecast/media/cma/filters/cma_renderer.cc:431: base::ResetAndReturn(&init_cb_).Run(error); On 2015/01/24 01:29:02, DaleCurtis wrote: > This may immediately destroy your class, so you can't do anything else > afterward. Ah --- in that case, I don't need the "initializing" flag, since we'll always bail out here.
https://codereview.chromium.org/869283003/diff/20001/chromecast/media/cma/fil... File chromecast/media/cma/filters/cma_renderer.cc (right): https://codereview.chromium.org/869283003/diff/20001/chromecast/media/cma/fil... chromecast/media/cma/filters/cma_renderer.cc:67: if (!flush_cb_.is_null()) On 2015/01/24 01:29:02, DaleCurtis wrote: > Should be an else here, Done.
https://codereview.chromium.org/869283003/diff/60001/chromecast/media/cma/fil... File chromecast/media/cma/filters/cma_renderer.cc (right): https://codereview.chromium.org/869283003/diff/60001/chromecast/media/cma/fil... chromecast/media/cma/filters/cma_renderer.cc:431: return; May be this is more readable? If (state_ != kError) { if (!init_cb_.is_null()) { base::ResetAnd....... return; } error_cb_.Run(error); }
https://codereview.chromium.org/869283003/diff/60001/chromecast/media/cma/fil... File chromecast/media/cma/filters/cma_renderer.cc (right): https://codereview.chromium.org/869283003/diff/60001/chromecast/media/cma/fil... chromecast/media/cma/filters/cma_renderer.cc:431: return; On 2015/01/26 07:48:53, erickung1 wrote: > May be this is more readable? > > If (state_ != kError) { > if (!init_cb_.is_null()) { > base::ResetAnd....... > return; > } > error_cb_.Run(error); > } Done.
On 2015/01/26 16:53:00, gunsch wrote: > https://codereview.chromium.org/869283003/diff/60001/chromecast/media/cma/fil... > File chromecast/media/cma/filters/cma_renderer.cc (right): > > https://codereview.chromium.org/869283003/diff/60001/chromecast/media/cma/fil... > chromecast/media/cma/filters/cma_renderer.cc:431: return; > On 2015/01/26 07:48:53, erickung1 wrote: > > May be this is more readable? > > > > If (state_ != kError) { > > if (!init_cb_.is_null()) { > > base::ResetAnd....... > > return; > > } > > error_cb_.Run(error); > > } > > Done. LGTM
https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... File chromecast/media/cma/filters/cma_renderer.cc (right): https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... chromecast/media/cma/filters/cma_renderer.cc:329: On 2015/01/24 19:29:16, gunsch wrote: > On 2015/01/24 01:24:06, DaleCurtis wrote: > > On 2015/01/24 01:20:19, gunsch wrote: > > > On 2015/01/24 01:01:56, DaleCurtis wrote: > > > > Initialize may still complete (successfully or not) after an OnError event > > > > occurs. So you need to check if init_cb_ is null / state_ != error like I > do > > > in > > > > my change. > > > > > > Hmm, not sure I follow --- if an OnError occurs during initialization, that > > > resets init_cb_ (as in your CL), in which case it seems to me that > > > initialization won't be able to finish. > > > > > > I did notice I missed an OnError call in OnAudioPipelineInitializeDone, > which > > > corresponds to one in your change as well. > > > > I don't know what your downstream looks like, but an OnError doesn't preclude > a > > downstream initialize callback from firing. I.e. On desktop, the audio > renderer > > may receive a device error at any time. > > Still not following. Are you describing either of the following sequences, and > if so can you clarify what I'm misunderstanding? > > Sequence 1 > * OnError fired _before_ Initialize is called > * Initialize won't be called, since Pipeline destroys Renderer > > Sequence 2 > * Initialize called, init_cb_ set > * A pipeline error occurs, OnError called > * OnError fires/resets init_cb_ with the error code, since state is not already > kError > * Initialize won't finish, since Pipeline destroys Renderer > > From my reading of Pipeline, both error_cb_ and init_cb_(error status) look like > they will destroy the Renderer instance. > > I've added the "state != error" check to OnError when calling init_cb_, but I > guess I'm still not seeing a sequence in which that would actually be necessary. Sequence1 isn't possible I think. Initialize() provides the error callback, so it can't notify the pipeline if some error happens before (what would?) For sequence 2, you're not guaranteed that destruction will occur immediately, so if you have outstanding callbacks they'll still fire. E.g., say audio initialized okay, but while video initialization is in progress an error is delivered by the audio renderer. Video initialization might still execute some callbacks if care isn't taken. Thus it seems like it'd be prudent to double check that the renderer isn't in an error state (like is done in the patch I submitted).
https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... File chromecast/media/cma/filters/cma_renderer.cc (right): https://codereview.chromium.org/869283003/diff/1/chromecast/media/cma/filters... chromecast/media/cma/filters/cma_renderer.cc:329: On 2015/01/26 18:42:28, DaleCurtis wrote: > On 2015/01/24 19:29:16, gunsch wrote: > > On 2015/01/24 01:24:06, DaleCurtis wrote: > > > On 2015/01/24 01:20:19, gunsch wrote: > > > > On 2015/01/24 01:01:56, DaleCurtis wrote: > > > > > Initialize may still complete (successfully or not) after an OnError > event > > > > > occurs. So you need to check if init_cb_ is null / state_ != error like > I > > do > > > > in > > > > > my change. > > > > > > > > Hmm, not sure I follow --- if an OnError occurs during initialization, > that > > > > resets init_cb_ (as in your CL), in which case it seems to me that > > > > initialization won't be able to finish. > > > > > > > > I did notice I missed an OnError call in OnAudioPipelineInitializeDone, > > which > > > > corresponds to one in your change as well. > > > > > > I don't know what your downstream looks like, but an OnError doesn't > preclude > > a > > > downstream initialize callback from firing. I.e. On desktop, the audio > > renderer > > > may receive a device error at any time. > > > > Still not following. Are you describing either of the following sequences, and > > if so can you clarify what I'm misunderstanding? > > > > Sequence 1 > > * OnError fired _before_ Initialize is called > > * Initialize won't be called, since Pipeline destroys Renderer > > > > Sequence 2 > > * Initialize called, init_cb_ set > > * A pipeline error occurs, OnError called > > * OnError fires/resets init_cb_ with the error code, since state is not > already > > kError > > * Initialize won't finish, since Pipeline destroys Renderer > > > > From my reading of Pipeline, both error_cb_ and init_cb_(error status) look > like > > they will destroy the Renderer instance. > > > > I've added the "state != error" check to OnError when calling init_cb_, but I > > guess I'm still not seeing a sequence in which that would actually be > necessary. > > Sequence1 isn't possible I think. Initialize() provides the error callback, so > it can't notify the pipeline if some error happens before (what would?) > > For sequence 2, you're not guaranteed that destruction will occur immediately, > so if you have outstanding callbacks they'll still fire. E.g., say audio > initialized okay, but while video initialization is in progress an error is > delivered by the audio renderer. Video initialization might still execute some > callbacks if care isn't taken. > > Thus it seems like it'd be prudent to double check that the renderer isn't in an > error state (like is done in the patch I submitted). Ahh, I see it now: moved CompleteStateTransition(kError) earlier in OnError, then bailing out both here and OnAudioPipelineInitializeDone if already in an error state. Thanks for the extra explanation.
lgtm
The CQ bit was checked by gunsch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/869283003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9ca5b72d0c04e2549d6374d86ef9ec7ac324010d Cr-Commit-Position: refs/heads/master@{#313195} |
