Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2605473002:
Fix processing of multiple stream status changes by renderer (Closed)
DescriptionFix processing of multiple stream status changes by renderer
Typically renderer needs to do Flush + StartPlaying in response to
stream status change (i.e. when stream gets enabled or disabled). But
since Flush is an async operation we might get another status change
of the same stream, while the renderer is still handling the previous
one. In the past renderer has simply ignored status changes while
another status change was processed. But that was problematic, since
we couldn't guarantee that the renderer status will be correct after
stream has been enabled/disabled very quickly a few times. This CL
fixes that issue by postponing status changes handling if necessary,
instead of dropping those notifications.
BUG=678031
Review-Url: https://codereview.chromium.org/2605473002
Cr-Commit-Position: refs/heads/master@{#442762}
Committed: https://chromium.googlesource.com/chromium/src/+/bff54a249b1e7e9fddc1740d980c0bafacf10970
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use a queue of postponed notifications + CR feedback #
Total comments: 6
Patch Set 3 : CR feedback #
Total comments: 2
Patch Set 4 : CR feedback (use a single unified notification queue) #
Messages
Total messages: 51 (31 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...
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...
Description was changed from ========== Fix processing of multiple stream status changes by renderer Typically renderer needs to do Flush + StartPlaying in response to stream status change (i.e. when stream gets enabled or disabled). But since Flush is an async operation we might get another status change of the same stream, while the renderer is still handling the previous one. In the past renderer has simply ignored status changes while another status change was processed. But that was problematic, since we couldn't guarantee that the renderer status will be correct after stream has been enabled/disabled very quickly a few times. This CL fixes that issue by postponing status changes handling if necessary, instead of dropping those notifications. BUG= ========== to ========== Fix processing of multiple stream status changes by renderer Typically renderer needs to do Flush + StartPlaying in response to stream status change (i.e. when stream gets enabled or disabled). But since Flush is an async operation we might get another status change of the same stream, while the renderer is still handling the previous one. In the past renderer has simply ignored status changes while another status change was processed. But that was problematic, since we couldn't guarantee that the renderer status will be correct after stream has been enabled/disabled very quickly a few times. This CL fixes that issue by postponing status changes handling if necessary, instead of dropping those notifications. BUG=678031 ==========
servolk@chromium.org changed reviewers: + avayvod@chromium.org, dalecurtis@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
avayvod@chromium.org changed reviewers: + xhwang@chromium.org
+Xiaohan who might be available (Dale is OOO for a couple of days AFAIK)
On 2017/01/03 23:01:08, whywhat wrote: > +Xiaohan who might be available (Dale is OOO for a couple of days AFAIK) I am not super familiar with the current logic. Wondering whether the following scenario is possible: 1. Stream status change (SSC) 1 arrived and is processed. 2. Before SSC 1 handling is finished, SSC 2 arrived and is delay posted (with a 10ms delay). 3. SSC 1 handling is finished after 1 ms. 4. SSC 3 arrived after 2 ms. Since there's no SSC handling pending, it's processed immediately. 5. SSC 3 handling is finished after 3 ms. 6. SSC 2 arrived after 10 ms as the result of (2). So now the SSC handling order is reversed. Is it possible and is it a concern? If so, is it possible to store pending SSCs in a list instead of posting tasks?
https://codereview.chromium.org/2605473002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2605473002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:37: static const int kStreamStatusHandlingDelayMs = 10; nit: no need to use "static" here. Namespace scope constant is already static. https://codereview.chromium.org/2605473002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2605473002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl_unittest.cc:758: base::TimeDelta time0; What does time0 exactly mean? Maybe this should be something like dummy_time to make it clear that the value doesn't matter.
On 2017/01/04 19:33:42, xhwang wrote: > On 2017/01/03 23:01:08, whywhat wrote: > > +Xiaohan who might be available (Dale is OOO for a couple of days AFAIK) > > I am not super familiar with the current logic. Wondering whether the following > scenario is possible: > > 1. Stream status change (SSC) 1 arrived and is processed. > 2. Before SSC 1 handling is finished, SSC 2 arrived and is delay posted (with a > 10ms delay). > 3. SSC 1 handling is finished after 1 ms. > 4. SSC 3 arrived after 2 ms. Since there's no SSC handling pending, it's > processed immediately. > 5. SSC 3 handling is finished after 3 ms. > 6. SSC 2 arrived after 10 ms as the result of (2). > > So now the SSC handling order is reversed. Is it possible and is it a concern? > If so, is it possible to store pending SSCs in a list instead of posting tasks? Yes, that's a good point, I think this could happen. I'll rework the fix to take this into account.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2605473002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2605473002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:37: static const int kStreamStatusHandlingDelayMs = 10; On 2017/01/04 19:34:02, xhwang wrote: > nit: no need to use "static" here. Namespace scope constant is already static. Done. https://codereview.chromium.org/2605473002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2605473002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl_unittest.cc:758: base::TimeDelta time0; On 2017/01/04 19:34:02, xhwang wrote: > What does time0 exactly mean? Maybe this should be something like dummy_time to > make it clear that the value doesn't matter. There's no special meaning to this, it's just a dummy value indeed and is slightly shorter to type than base::TimeDelta(). I guess we could just replace it with base::TimeDelta() to avoid introducing a variable, hopefully that's less confusing (and typing base::TimeDelta() is not that much easier than dummy_time anyway).
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/04 21:29:30, servolk wrote: > https://codereview.chromium.org/2605473002/diff/1/media/renderers/renderer_im... > File media/renderers/renderer_impl.cc (right): > > https://codereview.chromium.org/2605473002/diff/1/media/renderers/renderer_im... > media/renderers/renderer_impl.cc:37: static const int > kStreamStatusHandlingDelayMs = 10; > On 2017/01/04 19:34:02, xhwang wrote: > > nit: no need to use "static" here. Namespace scope constant is already static. > > Done. > > https://codereview.chromium.org/2605473002/diff/1/media/renderers/renderer_im... > File media/renderers/renderer_impl_unittest.cc (right): > > https://codereview.chromium.org/2605473002/diff/1/media/renderers/renderer_im... > media/renderers/renderer_impl_unittest.cc:758: base::TimeDelta time0; > On 2017/01/04 19:34:02, xhwang wrote: > > What does time0 exactly mean? Maybe this should be something like dummy_time > to > > make it clear that the value doesn't matter. > > There's no special meaning to this, it's just a dummy value indeed and is > slightly shorter to type than base::TimeDelta(). I guess we could just replace > it with base::TimeDelta() to avoid introducing a variable, hopefully that's less > confusing (and typing base::TimeDelta() is not that much easier than dummy_time > anyway). Ok, in the latest patchset I've switched to using a queue of postponed notifications and posting the next notification as soon as we are done processing the previous one. PTAL.
Thanks! This is better but I have some comments for discussion. https://codereview.chromium.org/2605473002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2605473002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:590: postponed_video_status_notifications_.front()); There's a chance where after this task (say SSC1) is posted but before that task is processed, OnStreamStatusChanged() is called again (say SSC2). Then SSC2 will be processed before SSC1. It might make sense to keep |restarting_video_| to be true and call OnStreamStatusChanged() directly here with the pending status. Or we could always push status changes in the queue then process them in order. Or anything else that works better :) https://codereview.chromium.org/2605473002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:591: postponed_video_status_notifications_.pop(); Does it make sense to return here and only proceed when we don't have any pending status changes? Actually, if we have multiple pending status changes, does it make sense to only process the last one, since all intermediate ones will be overwritten by the last one anyways I guess? https://codereview.chromium.org/2605473002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl.h (right): https://codereview.chromium.org/2605473002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.h:200: std::queue<base::Closure> postponed_audio_status_notifications_; tiny nit: - typically we use "pending" instead of "postponed"... but it's up to you. - *_audio_status_changes_ should be shorter and more consistent with *StatusChanged
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
https://codereview.chromium.org/2605473002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2605473002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:590: postponed_video_status_notifications_.front()); On 2017/01/04 22:04:16, xhwang wrote: > There's a chance where after this task (say SSC1) is posted but before that task > is processed, OnStreamStatusChanged() is called again (say SSC2). Then SSC2 will > be processed before SSC1. > > It might make sense to keep |restarting_video_| to be true and call > OnStreamStatusChanged() directly here with the pending status. Or we could > always push status changes in the queue then process them in order. Or anything > else that works better :) Yes, good catch. We need to set restarting_video_=false before calling OnStreamStatusChanged, since otherwise it will just postpone handling the status change. But what we can do here to prevent the problem you described is instead of setting restarting_video_ = false immediately, we can do it in a posted task and then call the pending status notification callback (if any) from the same posted task. This will ensure that the rest of the logic in the HandleRestartedStreamBufferingChanges and OnBufferingStateChange gets executed (thus putting us back into steady ongoing playback state) before we process the next pending notification. https://codereview.chromium.org/2605473002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:591: postponed_video_status_notifications_.pop(); On 2017/01/04 22:04:16, xhwang wrote: > Does it make sense to return here and only proceed when we don't have any > pending status changes? > > Actually, if we have multiple pending status changes, does it make sense to only > process the last one, since all intermediate ones will be overwritten by the > last one anyways I guess? I think it's better to do the opposite here, instead of returning right away, we should let the rest of the code in here and in OnBufferingStateChange to finish the work for putting the renderer back into playing state, when it's safe to start processing the next notification. Re multiple notifications: yes, we can probably try to handle only the last notification, but it needs to be done on a per-stream basis. I.e. if we have multiple audio or video streams we will still need to postpone handling an SSC notification for a different stream, we won't be able to get away with just a single pending SSC callback per each stream type. And once you start considering the case of multiple streams, it gets tricky. If we store pending callback in a map keyed by the demuxer stream, then how can we preserve the correct ordering of notifications between different streams of the same type. I guess I'll need to think more about this tomorrow, but for now having a queue seems a safer option. https://codereview.chromium.org/2605473002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl.h (right): https://codereview.chromium.org/2605473002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.h:200: std::queue<base::Closure> postponed_audio_status_notifications_; On 2017/01/04 22:04:16, xhwang wrote: > tiny nit: > - typically we use "pending" instead of "postponed"... but it's up to you. > - *_audio_status_changes_ should be shorter and more consistent with > *StatusChanged Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hmm, do we need a queue of these changes? Seems like we just want to track the latest one?
On 2017/01/05 22:48:26, DaleCurtis wrote: > Hmm, do we need a queue of these changes? Seems like we just want to track the > latest one? Well, as I've tried to explain in my previous comment, we could probably drop previous status change events for the same stream, but we still want to preserve the ordering of events for different streams of the same type, that's why I've kept event queues for now. I feel that preserving SSC ordering is more important than optimizing the number of events. In order to handle each notification all we need to do is just Flush + StartPlaying, so it's not that expensive anyway. But if we have let's say two video streams v1 and v2. And v2 was previously selected but now we are selecting v1, we want the notifications for v2 being disabled to be handled before v1 being enabled (since if we handle v1 being enabled first, the video renderer is still busy with rendering v2 and can't accomodate another video stream).
https://codereview.chromium.org/2605473002/diff/40001/media/renderers/rendere... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2605473002/diff/40001/media/renderers/rendere... media/renderers/renderer_impl.cc:223: if (restarting_video_) { Can restarting_video_ and restarting_audio_ be pulled outside of this conditional and coalesced into a single pending_status_notifications_ queue? Also, just use a vector for simplicity. You always insert at the end and when removing you remove everything. See also recent chromium-dev threads on avoiding dequeue/queue due to memory overhead.
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/2605473002/diff/40001/media/renderers/rendere... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2605473002/diff/40001/media/renderers/rendere... media/renderers/renderer_impl.cc:223: if (restarting_video_) { On 2017/01/10 21:14:48, DaleCurtis wrote: > Can restarting_video_ and restarting_audio_ be pulled outside of this > conditional and coalesced into a single pending_status_notifications_ queue? > Also, just use a vector for simplicity. You always insert at the end and when > removing you remove everything. See also recent chromium-dev threads on avoiding > dequeue/queue due to memory overhead. Yes, we can do this (uploaded a new patchset), although we do need to keep the two separate flags for restarting audio and video, in order to handle buffering status changes correctly (e.g. if we are restarting an audio stream, then a HAVE_NOTHING buffering status change on audio stream is expected. On the other hand if we get HAVE_NOTHING for video stream while restarting an audio stream, then we do want to report underflow). Re std::queue: I guess we can use std::list here instead. We do insert always at the end, but we remove items one-by-one from the front as we process all pending notifications.
Why do we need to remove items one by one from the front? Can't you just iterate through them all and then clear the entire list?
On 2017/01/11 00:01:00, DaleCurtis wrote: > Why do we need to remove items one by one from the front? Can't you just iterate > through them all and then clear the entire list? Because in order to properly handle each stream status change notification we need to do Flush + StartPlaying on the corresponding renderer, and Flush is an async operation. We need to do a Flush when enabling the stream in order to ensure that the corresponding renderer gets out of its <ended> state and it ready for rendering more data. And we need to do a Flush when disabling a stream so that the effect of stream being disabled is instantaneous and we don't wait for the pipeline to get to and process the EOS buffer (which might take a while on platforms with deep rendering pipelines, like Chromecast, see crbug.com/633299).
OIC, you're popping them one at a time. std::list is fine, or just std::swap(back)+pop if you want to stick with the vector. The typical erase-remove idiom would handle that. lgtm
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.
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2605473002/#ps60001 (title: "CR feedback (use a single unified notification queue)")
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": 1484098260360730,
"parent_rev": "a94e1212122cbd16cab63f2d10ed9e8a74a8fe98", "commit_rev":
"bff54a249b1e7e9fddc1740d980c0bafacf10970"}
Message was sent while issue was closed.
Description was changed from ========== Fix processing of multiple stream status changes by renderer Typically renderer needs to do Flush + StartPlaying in response to stream status change (i.e. when stream gets enabled or disabled). But since Flush is an async operation we might get another status change of the same stream, while the renderer is still handling the previous one. In the past renderer has simply ignored status changes while another status change was processed. But that was problematic, since we couldn't guarantee that the renderer status will be correct after stream has been enabled/disabled very quickly a few times. This CL fixes that issue by postponing status changes handling if necessary, instead of dropping those notifications. BUG=678031 ========== to ========== Fix processing of multiple stream status changes by renderer Typically renderer needs to do Flush + StartPlaying in response to stream status change (i.e. when stream gets enabled or disabled). But since Flush is an async operation we might get another status change of the same stream, while the renderer is still handling the previous one. In the past renderer has simply ignored status changes while another status change was processed. But that was problematic, since we couldn't guarantee that the renderer status will be correct after stream has been enabled/disabled very quickly a few times. This CL fixes that issue by postponing status changes handling if necessary, instead of dropping those notifications. BUG=678031 Review-Url: https://codereview.chromium.org/2605473002 Cr-Commit-Position: refs/heads/master@{#442762} Committed: https://chromium.googlesource.com/chromium/src/+/bff54a249b1e7e9fddc1740d980c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bff54a249b1e7e9fddc1740d980c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
