Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2855373002:
Use ffmpeg packet.pos for restarting reading after reenabling video
DescriptionUse ffmpeg packet.pos for restarting reading after reenabling video
FFmpeg has a single file read position shared between all active
sub-streams. But when we are restarting an individual sub-stream we'd
like to seek and read previous buffers only for that restarted stream,
without duplicating the already seen packets for other substreams.
Luckily FFmpeg's AVPacket has a pos field, which indicates the file
position that packet was read from. So when we need to restart a video
stream, we can do a seek backward in the file and then drop packets for
other streams until we reach a previous read position, and at that
point we'll restart reading all streams.
BUG=709302
Patch Set 1 #Patch Set 2 : fixed audio rewinding #
Total comments: 15
Patch Set 3 : nits #Patch Set 4 : Refactor some common code into SeekInternal #Patch Set 5 : Extract some more seek logic and add comments for SeekInternal #
Total comments: 10
Patch Set 6 : Handle track re-enabling during seek and seek initiated during track re-enabling #Patch Set 7 : Added some unit tests + some fixes #Patch Set 8 : rebase #Patch Set 9 : Reset pending_seek_position_ in AbortPendingReads (fixes test failures) #Patch Set 10 : reset pending_seek_position_ after calling the pending_seek_cb_ #
Total comments: 14
Messages
Total messages: 62 (38 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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use ffmpeg packet.pos for restarting reading after reenabling video ========== to ========== Use ffmpeg packet.pos for restarting reading after reenabling video FFmpeg has a single file read position shared between all active sub-streams. But when we are restarting an individual sub-stream we'd like to seek and read previous buffers only for that restarted stream, without duplicating the already seen packets for other substreams. Luckily FFmpeg's AVPacket has a pos field, which indicates the file position that packet was read from. So when we need to restart a video stream, we can do a seek backward in the file and then drop packets for other streams until we reach a previous read position, and at that point we'll restart reading all streams. BUG=709302 ==========
servolk@chromium.org changed reviewers: + avayvod@chromium.org, dalecurtis@chromium.org
dalecurtis@chromium.org changed reviewers: + xhwang@chromium.org
looking good, but needs some tests. +xhwang since you probably won't have the tests ready before I head out. https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1737: base::TimeDelta seek_time = Extract common code from Seek() and put in an internal helper which we can use here. Perhaps it can take a done_cb to reuse almost all the code too. https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1764: restarting_stream_ = stream; What happens if another track is enabled and tramples restarting_stream_ before we finish the track change? Are we guaranteed this won't happen? https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1856: bool drop_seen_packet = Drop unnecessary parens. https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1858: last_packet_pos_ >= 0 && packet.get()->pos <= last_packet_pos_); Is pos monotonically increasing? I.e. this won't be changed by badly muxed files? https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1860: DVLOG(4) << "Dropping packet: pos=" << packet.get()->pos; "already seen packet"
https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1737: base::TimeDelta seek_time = On 2017/05/04 18:53:03, DaleCurtis_OOO_May_5_To_May23 wrote: > Extract common code from Seek() and put in an internal helper which we can use > here. Perhaps it can take a done_cb to reuse almost all the code too. Will do. https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1764: restarting_stream_ = stream; On 2017/05/04 18:53:03, DaleCurtis_OOO_May_5_To_May23 wrote: > What happens if another track is enabled and tramples restarting_stream_ before > we finish the track change? Are we guaranteed this won't happen? Great question. I think it could happen, but we should be fine. If we got here when the restarting_stream_ is non-null, it means that we are still in the 'stream restart' mode, i.e. we are reading packets for only one stream (the current restarting_stream_) and dropping packets for other streams. We'll simply remain in the stream restart mode, but now we'll be reading only packets for the new |stream|. if stream == restarting_stream_, then it means we've already done FlushBuffers in the OnSelectedVideoStream changed. Otherwise I guess there might be some packets left in the previous restarting stream, but it shouldn't interfere with anything, only slightly affect memory usage. I guess we can do restarting_stream_->FlushBuffers (if restarting_stream_ is non-null already) here before overwriting it, to make a small improvement. But it shouldn't affect playback behavior. https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1856: bool drop_seen_packet = On 2017/05/04 18:53:03, DaleCurtis_OOO_May_5_To_May23 wrote: > Drop unnecessary parens. Done. https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1858: last_packet_pos_ >= 0 && packet.get()->pos <= last_packet_pos_); On 2017/05/04 18:53:03, DaleCurtis_OOO_May_5_To_May23 wrote: > Is pos monotonically increasing? I.e. this won't be changed by badly muxed > files? I don't know for sure, but I think it should be monotonically increasing even for badly muxed files. IIUC timestamps (pts) might be all over the place for badly muxed files, but file read position will still increase monotonically. https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1860: DVLOG(4) << "Dropping packet: pos=" << packet.get()->pos; On 2017/05/04 18:53:03, DaleCurtis_OOO_May_5_To_May23 wrote: > "already seen packet" Done.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1764: restarting_stream_ = stream; On 2017/05/04 at 19:15:16, servolk wrote: > On 2017/05/04 18:53:03, DaleCurtis_OOO_May_5_To_May23 wrote: > > What happens if another track is enabled and tramples restarting_stream_ before > > we finish the track change? Are we guaranteed this won't happen? > > Great question. I think it could happen, but we should be fine. If we got here when the restarting_stream_ is non-null, it means that we are still in the 'stream restart' mode, i.e. we are reading packets for only one stream (the current restarting_stream_) and dropping packets for other streams. We'll simply remain in the stream restart mode, but now we'll be reading only packets for the new |stream|. if stream == restarting_stream_, then it means we've already done FlushBuffers in the OnSelectedVideoStream changed. Otherwise I guess there might be some packets left in the previous restarting stream, but it shouldn't interfere with anything, only slightly affect memory usage. I guess we can do restarting_stream_->FlushBuffers (if restarting_stream_ is non-null already) here before overwriting it, to make a small improvement. But it shouldn't affect playback behavior. Isn't that wrong though? I.e. the stream we restarted first may have 0 or more packets, so we need to deliver all packets to that stream, but only restarting_stream gets the special logic to ensure it gets all packets.
https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1764: restarting_stream_ = stream; On 2017/05/04 21:21:06, DaleCurtis_OOO_May_5_To_May23 wrote: > On 2017/05/04 at 19:15:16, servolk wrote: > > On 2017/05/04 18:53:03, DaleCurtis_OOO_May_5_To_May23 wrote: > > > What happens if another track is enabled and tramples restarting_stream_ > before > > > we finish the track change? Are we guaranteed this won't happen? > > > > Great question. I think it could happen, but we should be fine. If we got here > when the restarting_stream_ is non-null, it means that we are still in the > 'stream restart' mode, i.e. we are reading packets for only one stream (the > current restarting_stream_) and dropping packets for other streams. We'll simply > remain in the stream restart mode, but now we'll be reading only packets for the > new |stream|. if stream == restarting_stream_, then it means we've already done > FlushBuffers in the OnSelectedVideoStream changed. Otherwise I guess there might > be some packets left in the previous restarting stream, but it shouldn't > interfere with anything, only slightly affect memory usage. I guess we can do > restarting_stream_->FlushBuffers (if restarting_stream_ is non-null already) > here before overwriting it, to make a small improvement. But it shouldn't affect > playback behavior. > > Isn't that wrong though? I.e. the stream we restarted first may have 0 or more > packets, so we need to deliver all packets to that stream, but only > restarting_stream gets the special logic to ensure it gets all packets. I think that's ok, because we only use this logic for video streams and there can be only one selected video stream at a time. So we need to handle only two cases: 1. Video stream A was deselected (disabled), then re-enabled, at that point we'll enter the restarting mode and will start reading packets for it, then the same stream A gets disabled and re-enabled again before we finished handling the first operation. In this case we should be fine, because we did FlushBuffers on stream A when it got re-enabled the second time, and now we've just done a seek to a new restart position and will resume reading more frames for stream A from the new restart position. There's no need to deliver old frames as we never finished the first restart, and they would have been dropped by the video renderer anyway (because it also does a flush every time a stream is restarted). 2. If we have more than one video stream, let's say we have video streams A and B. First the stream A gets re-enabled and we start buffering data for it. But then the selected video stream is changed to B. Now we don't need to finish delivering any read packets for A, because A is automatically deselected/disabled when the video stream B is selected.
https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1764: restarting_stream_ = stream; On 2017/05/04 at 21:38:30, servolk wrote: > On 2017/05/04 21:21:06, DaleCurtis_OOO_May_5_To_May23 wrote: > > On 2017/05/04 at 19:15:16, servolk wrote: > > > On 2017/05/04 18:53:03, DaleCurtis_OOO_May_5_To_May23 wrote: > > > > What happens if another track is enabled and tramples restarting_stream_ > > before > > > > we finish the track change? Are we guaranteed this won't happen? > > > > > > Great question. I think it could happen, but we should be fine. If we got here > > when the restarting_stream_ is non-null, it means that we are still in the > > 'stream restart' mode, i.e. we are reading packets for only one stream (the > > current restarting_stream_) and dropping packets for other streams. We'll simply > > remain in the stream restart mode, but now we'll be reading only packets for the > > new |stream|. if stream == restarting_stream_, then it means we've already done > > FlushBuffers in the OnSelectedVideoStream changed. Otherwise I guess there might > > be some packets left in the previous restarting stream, but it shouldn't > > interfere with anything, only slightly affect memory usage. I guess we can do > > restarting_stream_->FlushBuffers (if restarting_stream_ is non-null already) > > here before overwriting it, to make a small improvement. But it shouldn't affect > > playback behavior. > > > > Isn't that wrong though? I.e. the stream we restarted first may have 0 or more > > packets, so we need to deliver all packets to that stream, but only > > restarting_stream gets the special logic to ensure it gets all packets. > > I think that's ok, because we only use this logic for video streams and there can be only one selected video stream at a time. So we need to handle only two cases: > 1. Video stream A was deselected (disabled), then re-enabled, at that point we'll enter the restarting mode and will start reading packets for it, then the same stream A gets disabled and re-enabled again before we finished handling the first operation. In this case we should be fine, because we did FlushBuffers on stream A when it got re-enabled the second time, and now we've just done a seek to a new restart position and will resume reading more frames for stream A from the new restart position. There's no need to deliver old frames as we never finished the first restart, and they would have been dropped by the video renderer anyway (because it also does a flush every time a stream is restarted). > 2. If we have more than one video stream, let's say we have video streams A and B. First the stream A gets re-enabled and we start buffering data for it. But then the selected video stream is changed to B. Now we don't need to finish delivering any read packets for A, because A is automatically deselected/disabled when the video stream B is selected. Even if we just consider audio, depending on how the file is muxed if you drop packets like this you are not guaranteed to have audio for the time you need. I.e. with a bad muxing the audio packets you may need for the video time T are before last packet pos and thus get dropped.
https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1764: restarting_stream_ = stream; On 2017/05/04 at 21:41:12, DaleCurtis_OOO_May_5_To_May23 wrote: > On 2017/05/04 at 21:38:30, servolk wrote: > > On 2017/05/04 21:21:06, DaleCurtis_OOO_May_5_To_May23 wrote: > > > On 2017/05/04 at 19:15:16, servolk wrote: > > > > On 2017/05/04 18:53:03, DaleCurtis_OOO_May_5_To_May23 wrote: > > > > > What happens if another track is enabled and tramples restarting_stream_ > > > before > > > > > we finish the track change? Are we guaranteed this won't happen? > > > > > > > > Great question. I think it could happen, but we should be fine. If we got here > > > when the restarting_stream_ is non-null, it means that we are still in the > > > 'stream restart' mode, i.e. we are reading packets for only one stream (the > > > current restarting_stream_) and dropping packets for other streams. We'll simply > > > remain in the stream restart mode, but now we'll be reading only packets for the > > > new |stream|. if stream == restarting_stream_, then it means we've already done > > > FlushBuffers in the OnSelectedVideoStream changed. Otherwise I guess there might > > > be some packets left in the previous restarting stream, but it shouldn't > > > interfere with anything, only slightly affect memory usage. I guess we can do > > > restarting_stream_->FlushBuffers (if restarting_stream_ is non-null already) > > > here before overwriting it, to make a small improvement. But it shouldn't affect > > > playback behavior. > > > > > > Isn't that wrong though? I.e. the stream we restarted first may have 0 or more > > > packets, so we need to deliver all packets to that stream, but only > > > restarting_stream gets the special logic to ensure it gets all packets. > > > > I think that's ok, because we only use this logic for video streams and there can be only one selected video stream at a time. So we need to handle only two cases: > > 1. Video stream A was deselected (disabled), then re-enabled, at that point we'll enter the restarting mode and will start reading packets for it, then the same stream A gets disabled and re-enabled again before we finished handling the first operation. In this case we should be fine, because we did FlushBuffers on stream A when it got re-enabled the second time, and now we've just done a seek to a new restart position and will resume reading more frames for stream A from the new restart position. There's no need to deliver old frames as we never finished the first restart, and they would have been dropped by the video renderer anyway (because it also does a flush every time a stream is restarted). > > 2. If we have more than one video stream, let's say we have video streams A and B. First the stream A gets re-enabled and we start buffering data for it. But then the selected video stream is changed to B. Now we don't need to finish delivering any read packets for A, because A is automatically deselected/disabled when the video stream B is selected. > > Even if we just consider audio, depending on how the file is muxed if you drop packets like this you are not guaranteed to have audio for the time you need. I.e. with a bad muxing the audio packets you may need for the video time T are before last packet pos and thus get dropped. Oh right I see what you mean, we only run the seek on video so should be okay on audio packets since their buffers are guaranteed to contain the current time. This is a lot of assumptions that need to be documented and have tests / DCHECKS present.
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/2855373002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1737: base::TimeDelta seek_time = On 2017/05/04 19:15:16, servolk wrote: > On 2017/05/04 18:53:03, DaleCurtis_OOO_May_5_To_May23 wrote: > > Extract common code from Seek() and put in an internal helper which we can use > > here. Perhaps it can take a done_cb to reuse almost all the code too. > > Will do. Done (I've extracted most of the common seeking logic into SeekInternal in patchset #3).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wolenetz@chromium.org changed reviewers: + sandersd@chromium.org, wolenetz@chromium.org
Some comments added. +sandersd@ due to Dale OoO and sandersd@ also reviewing previous ffmpeg-read-canceling changes. Tests are needed, too. https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1695: SeekInternal(curr_time, selected_stream, could we already be in the middle of either a normal seek or another deselect->reselect video track seek? If so, it seems this needs to protected against (no more that 1 seek of any kind should be allowed to be in progress against the ffmpeg API). https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1701: void FFmpegDemuxer::OnSeekDoneForRestartingStream(FFmpegDemuxerStream* stream, What if ::Stop() occurs while this seek was in progress? Also, what if this FFmpegDemuxer is destructed prior to this callback occurring? There's quite a bit of machinery already in the normal ::Seek() process to handle such problems; similar is likely necessary for these "restart" seeks. Would re-using the same machinery be feasible? https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1743: if (stopped_ || !pending_seek_cb_.is_null()) probably need to also cancel the read result that arrives while a restart-seek is in progress https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1806: last_packet_pos_ >= 0 && packet.get()->pos <= last_packet_pos_; Is pos non-decreasing (during normal sequence of reads, and can only go backwards on seek), and on the same dimension across all streams (do all streams share a common pos?) https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.h:14: // NOTE: since FFmpegDemuxer reads packets sequentially without seeking, media this comment seems a bit incorrect now with this change.
No additional concerns beyond what wolenetz@ already commented on. https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1705: DVLOG(1) << __func__ << ": seek failed: " << AVErrorToString(result); Looks like this should probably be logged to the media log?
https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1695: SeekInternal(curr_time, selected_stream, On 2017/05/08 20:55:33, wolenetz_ooo_May_10 wrote: > could we already be in the middle of either a normal seek or another > deselect->reselect video track seek? If so, it seems this needs to protected > against (no more that 1 seek of any kind should be allowed to be in progress > against the ffmpeg API). Yeah, I looked closer at how FFmpegDemuxer is called and looks like it's possible, which is unfortunate, since we'll need to make the logic here more complicated to handle this. I think we need to consider two cases here: 1. A track is re-enabled while we have a pending seek in progress. In this case we'll need to re-start the pending seek to the same position immediately after it completes, since the newly enabled stream will have its avdiscard flags updated and the actual seek target position in the input stream might change. 2. If seek is initiated while the video stream restart is in progress (i.e. the |restarting_stream_| is not null and we are still reading past data for the video stream). In this case I believe we only need to reset the restarting_stream_ to null and proceed with the seek. The reading is guaranteed to restart from the correct file position for the new seek_target, because by that point the avdiscard flags for the re-enabled streams are updated. I'll see if I can find a way to add new test cases for these. https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1701: void FFmpegDemuxer::OnSeekDoneForRestartingStream(FFmpegDemuxerStream* stream, On 2017/05/08 20:55:33, wolenetz_ooo_May_10 wrote: > What if ::Stop() occurs while this seek was in progress? Also, what if this > FFmpegDemuxer is destructed prior to this callback occurring? There's quite a > bit of machinery already in the normal ::Seek() process to handle such problems; > similar is likely necessary for these "restart" seeks. Would re-using the same > machinery be feasible? I believe the destruction of FFmpegDemuxer should be ok due to using weak_ptrs here (any pending callbacks will just be canceled). Re. ::Stop - that's a good question, but I not sure what machinery you are referring to, could you point me to it please? I don't see pending_seek_cb_ being checked in FFmpegDemuxerStream::Stop. Should there be at least a DCHECK(!pending_seek_cb_) perhaps? https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1705: DVLOG(1) << __func__ << ": seek failed: " << AVErrorToString(result); On 2017/05/10 22:27:49, sandersd wrote: > Looks like this should probably be logged to the media log? Done. https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1806: last_packet_pos_ >= 0 && packet.get()->pos <= last_packet_pos_; On 2017/05/08 20:55:33, wolenetz_ooo_May_10 wrote: > Is pos non-decreasing (during normal sequence of reads, and can only go > backwards on seek), and on the same dimension across all streams (do all streams > share a common pos?) IIUC the answer is yes to both questions. AVPacket::pos is the byte position of the AVPacket in stream (see https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavcodec/avcodec.h?...). AFAIK FFmpeg reads packets from the stream sequentially in their bytestream order. That's why we chose to use it here (I'll add you to the mail thread where we discussed this with Dale).
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/12 00:55:37, servolk wrote: > https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... > File media/filters/ffmpeg_demuxer.cc (right): > > https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... > media/filters/ffmpeg_demuxer.cc:1695: SeekInternal(curr_time, selected_stream, > On 2017/05/08 20:55:33, wolenetz_ooo_May_10 wrote: > > could we already be in the middle of either a normal seek or another > > deselect->reselect video track seek? If so, it seems this needs to protected > > against (no more that 1 seek of any kind should be allowed to be in progress > > against the ffmpeg API). > > Yeah, I looked closer at how FFmpegDemuxer is called and looks like it's > possible, which is unfortunate, since we'll need to make the logic here more > complicated to handle this. I think we need to consider two cases here: > 1. A track is re-enabled while we have a pending seek in progress. In this case > we'll need to re-start the pending seek to the same position immediately after > it completes, since the newly enabled stream will have its avdiscard flags > updated and the actual seek target position in the input stream might change. > 2. If seek is initiated while the video stream restart is in progress (i.e. the > |restarting_stream_| is not null and we are still reading past data for the > video stream). In this case I believe we only need to reset the > restarting_stream_ to null and proceed with the seek. The reading is guaranteed > to restart from the correct file position for the new seek_target, because by > that point the avdiscard flags for the re-enabled streams are updated. > > I'll see if I can find a way to add new test cases for these. > > https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... > media/filters/ffmpeg_demuxer.cc:1701: void > FFmpegDemuxer::OnSeekDoneForRestartingStream(FFmpegDemuxerStream* stream, > On 2017/05/08 20:55:33, wolenetz_ooo_May_10 wrote: > > What if ::Stop() occurs while this seek was in progress? Also, what if this > > FFmpegDemuxer is destructed prior to this callback occurring? There's quite a > > bit of machinery already in the normal ::Seek() process to handle such > problems; > > similar is likely necessary for these "restart" seeks. Would re-using the same > > machinery be feasible? > > I believe the destruction of FFmpegDemuxer should be ok due to using weak_ptrs > here (any pending callbacks will just be canceled). > Re. ::Stop - that's a good question, but I not sure what machinery you are > referring to, could you point me to it please? I don't see pending_seek_cb_ > being checked in FFmpegDemuxerStream::Stop. Should there be at least a > DCHECK(!pending_seek_cb_) perhaps? > > https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... > media/filters/ffmpeg_demuxer.cc:1705: DVLOG(1) << __func__ << ": seek failed: " > << AVErrorToString(result); > On 2017/05/10 22:27:49, sandersd wrote: > > Looks like this should probably be logged to the media log? > > Done. > > https://codereview.chromium.org/2855373002/diff/80001/media/filters/ffmpeg_de... > media/filters/ffmpeg_demuxer.cc:1806: last_packet_pos_ >= 0 && packet.get()->pos > <= last_packet_pos_; > On 2017/05/08 20:55:33, wolenetz_ooo_May_10 wrote: > > Is pos non-decreasing (during normal sequence of reads, and can only go > > backwards on seek), and on the same dimension across all streams (do all > streams > > share a common pos?) > > IIUC the answer is yes to both questions. > AVPacket::pos is the byte position of the AVPacket in stream (see > https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavcodec/avcodec.h?...). > AFAIK FFmpeg reads packets from the stream sequentially in their bytestream > order. That's why we chose to use it here (I'll add you to the mail thread where > we discussed this with Dale). Ping. I've added unit tests and made some small fixes and the latest patchset is green on the CQ, so PTAL.
https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:55: const base::TimeDelta kDefaultStreamCapacity = base::TimeDelta::FromSeconds(2); constexpr or this is a static initializer. https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:1733: } else { Lets avoid this whole issue by just not allowing track changes until the seek completes. We do not allow multiple concurrent operations of other types for simplicity. I think that's one of the big issues we've had with track changes thus far; they should be queued after any pending operations instead of taking affect immediately. I.e., this queuing should happen in the Pipeline, not here. https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:1866: if (!restarting_stream_ && demuxer_stream->type() == DemuxerStream::AUDIO) Isn't this going to be incorrect for streams with embedded text tracks? I.e. we'll end up duplicating some of the text packets? https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:1973: // TODO(dalecurtis): Currently FFmpeg does not ensure that all streams in a Note we may run into this issue now more frequently... https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.h:316: const FFmpegSeekDoneCB& ffmpeg_seek_done_cb); pass by value nowadays. https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.h:380: const PipelineStatusCB& cb, Ditto.
https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:55: const base::TimeDelta kDefaultStreamCapacity = base::TimeDelta::FromSeconds(2); On 2017/05/24 22:24:17, DaleCurtis wrote: > constexpr or this is a static initializer. Done. https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:1733: } else { On 2017/05/24 22:24:18, DaleCurtis wrote: > Lets avoid this whole issue by just not allowing track changes until the seek > completes. We do not allow multiple concurrent operations of other types for > simplicity. > > I think that's one of the big issues we've had with track changes thus far; they > should be queued after any pending operations instead of taking affect > immediately. I.e., this queuing should happen in the Pipeline, not here. Sure, if we didn't allow track changes during pending seeks that would make our life (and this code) easier. But I think we need to consider a few points first: 1. Wouldn't that be counter to HTML5 spec? Track changes and seeks are initiated by Javascript code and I'm not aware of any place in the html5 spec that says that track changes during pending seeks are prohibited. On one hand it might be common sense, but on the other hand apps might attempt that and we need to be prepared to deal with it. 2. If we still decide to disallow it, how exactly should we do this? Postpone track changes on the PipelineImpl level until a pending seek is completed? Ignore (i.e. drop) track changes when there's a pending seek? DCHECK? https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:1973: // TODO(dalecurtis): Currently FFmpeg does not ensure that all streams in a On 2017/05/24 22:24:18, DaleCurtis wrote: > Note we may run into this issue now more frequently... We might, especially considering that this is going to be triggered whenever a background tab with background video track optimization comes into foreground. But can we do anything to mitigate this issue? I've read most of that FFmpeg mailing list thread, but it's not clear to me if anything has actually been done in FFmpeg to fix this issue. Looks like there's been some discussion between FFmpeg folks, but no further activity in that thread after Aaron's message. Would we need to switch to avformat_seek_file to fix this? Is that API now stable in FFmpeg? Anyway, whatever we'd need to do to address this would probably need to be a separate CL, since it sounds non-trivial. https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.h:316: const FFmpegSeekDoneCB& ffmpeg_seek_done_cb); On 2017/05/24 22:24:18, DaleCurtis wrote: > pass by value nowadays. Done.
https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:1733: } else { On 2017/05/24 at 22:58:19, servolk wrote: > On 2017/05/24 22:24:18, DaleCurtis wrote: > > Lets avoid this whole issue by just not allowing track changes until the seek > > completes. We do not allow multiple concurrent operations of other types for > > simplicity. > > > > I think that's one of the big issues we've had with track changes thus far; they > > should be queued after any pending operations instead of taking affect > > immediately. I.e., this queuing should happen in the Pipeline, not here. > > Sure, if we didn't allow track changes during pending seeks that would make our life (and this code) easier. But I think we need to consider a few points first: > 1. Wouldn't that be counter to HTML5 spec? Track changes and seeks are initiated by Javascript code and I'm not aware of any place in the html5 spec that says that track changes during pending seeks are prohibited. On one hand it might be common sense, but on the other hand apps might attempt that and we need to be prepared to deal with it. > 2. If we still decide to disallow it, how exactly should we do this? Postpone track changes on the PipelineImpl level until a pending seek is completed? Ignore (i.e. drop) track changes when there's a pending seek? DCHECK? We're not talking about prohibiting them, we're talking about queuing them at a different layer. Similar to how PipelineController keeps track of a pending seek time. Seeks have always been async, you fire it is not complete until the seeked event. All I'm proposing here is that this is the wrong layer at which to be handling the queuing, instead the pipeline should be delaying this track change until the seek completes or delaying the seek until the track change completes.
On 2017/05/24 23:15:46, DaleCurtis wrote: > https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... > File media/filters/ffmpeg_demuxer.cc (right): > > https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... > media/filters/ffmpeg_demuxer.cc:1733: } else { > On 2017/05/24 at 22:58:19, servolk wrote: > > On 2017/05/24 22:24:18, DaleCurtis wrote: > > > Lets avoid this whole issue by just not allowing track changes until the > seek > > > completes. We do not allow multiple concurrent operations of other types for > > > simplicity. > > > > > > I think that's one of the big issues we've had with track changes thus far; > they > > > should be queued after any pending operations instead of taking affect > > > immediately. I.e., this queuing should happen in the Pipeline, not here. > > > > Sure, if we didn't allow track changes during pending seeks that would make > our life (and this code) easier. But I think we need to consider a few points > first: > > 1. Wouldn't that be counter to HTML5 spec? Track changes and seeks are > initiated by Javascript code and I'm not aware of any place in the html5 spec > that says that track changes during pending seeks are prohibited. On one hand it > might be common sense, but on the other hand apps might attempt that and we need > to be prepared to deal with it. > > 2. If we still decide to disallow it, how exactly should we do this? Postpone > track changes on the PipelineImpl level until a pending seek is completed? > Ignore (i.e. drop) track changes when there's a pending seek? DCHECK? > > We're not talking about prohibiting them, we're talking about queuing them at a > different layer. Similar to how PipelineController keeps track of a pending seek > time. > > Seeks have always been async, you fire it is not complete until the seeked > event. All I'm proposing here is that this is the wrong layer at which to be > handling the queuing, instead the pipeline should be delaying this track change > until the seek completes or delaying the seek until the track change completes. Ok, yes, I agree we can probably try to ensure on the pipeline controller level that seeks and track status changes never overlap. Though it's going to be a non-trivial change, because we are currently not notifying the pipeline when the track status change handling is complete, so this will require some changes in the renderer. What should we do about this CL though? The main objective of this CL was to speed up video restarts of a video track is re-enabled by performing that internal seek and dropping of previously seen ffmpeg packets. Would you be ok with removing the logic for handling the various combinations of pending seeks and track status changes from this CL (probably replace with DCHECKs) and landing it like that for now and then making the necessary changes to ensure non-overlapping seeks/track changes in pipeline_controller in a separate CL?
I don't think there's any rush to land this half-done; we won't be able to enable it for users until seeks and track changes are mutually exclusive. Lets focus on getting the PipelineController to do the right thing when pending seeks or track changes are ongoing. Adding callbacks for track change completion should be fairly trivial (they already exist, they just aren't forwarded outside renderer), so the rest of the issue is just updating the logic in PipelineController to do what it does for seeks. https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:1641: // If we had been restarting the video stream, we can cancel that now, because I think this isn't true since you don't know how far along the seek is by the time of enable; the enable must occur before the av_seek_frame() call. You can only clear this if a user-initiated seek comes in while we're trying to seek for a restart. https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:1758: if (restarting_stream_) This will need an early exit if a seek() comes in during the restart for now. https://codereview.chromium.org/2855373002/diff/180001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:1973: // TODO(dalecurtis): Currently FFmpeg does not ensure that all streams in a On 2017/05/24 at 22:58:19, servolk wrote: > On 2017/05/24 22:24:18, DaleCurtis wrote: > > Note we may run into this issue now more frequently... > > We might, especially considering that this is going to be triggered whenever a background tab with background video track optimization comes into foreground. But can we do anything to mitigate this issue? > I've read most of that FFmpeg mailing list thread, but it's not clear to me if anything has actually been done in FFmpeg to fix this issue. Looks like there's been some discussion between FFmpeg folks, but no further activity in that thread after Aaron's message. Would we need to switch to avformat_seek_file to fix this? Is that API now stable in FFmpeg? > Anyway, whatever we'd need to do to address this would probably need to be a separate CL, since it sounds non-trivial. I don't think that API fixes the issue. I experimented with it. It's unclear what would need to be done, so we'll just have to see what happens. I think for video it's not an issue, we'll always get a key frame, but audio or text tracks may be stuck at a different point.
+Mounir for context
What's up with this servolk? Did you run into some issues with this approach?
On 2017/06/14 00:30:57, DaleCurtis wrote: > What's up with this servolk? Did you run into some issues with this approach? Yeah, sorry for the delay on this, I did actually run into an issue that I wasn't sure how to solve best, and then I got some Chromecast work that took precedence over this, so I was planning to get back to this CL when I got more time to think about how to properly solve it. So the issue is actually with this (quoting what you said in comment #55): > Adding callbacks for track change completion should be fairly trivial > (they already exist, they just aren't forwarded outside renderer) This turned out to be not so trivial after all. Here is the problem: When a video track change is changed, the change is processed by the demuxer first (PipelineController::OnSelectedVideoTrackChanged -> PipelineImpl::OnSelectedVideoTrackChanged -> PipelineImpl::RendererWrapper::OnSelectedVideoTrackChanged -> demuxer_->OnSelectedVideoTrackChanged), and now we want to be notified in the PipelineImpl/PipelineController when the track status processing has been completed, right? How can we do it? I see two options: 1. We pass a track status completion callback into the demuxer_->OnSelectedVideoTrackChanged. But that's not gonna work, because the demuxer only knows when the track change has started, it doesn't know when the track status change processing completes (that is known in the renderer), so we'd need to either somehow pass the completion callback on to the renderer, which could probably be done, but the completion cb should probably be a movable-only base::OnceCallback in the new callback system, and potentially a track change affects multiple demuxer streams (e.g. when switching video tracks one of the video stream becomes disabled, another one becomes enabled), so we'd need to decide if we need a separate callback for each stream status processing being finished on the level of the Demuxer-Renderer interfaces, or just use the last stream status with non-null CB. It's not really a trivial change and would need to updates to the RendererImpl logic, I haven't though this through fully yet. 2. We could pass a track status completion callback from RendererWrapper directly to the Renderer. The Renderer knows when the track status changes are completed, so it could invoke that callback, no problem, but the problem is how would we pass a callback from the RendererWrapper into the renderer? The RendererWrapper operates with a generic media::Renderer interface, not the RendererImpl. So we'd need to introduce a new method into the media::Renderer interface for setting the track status completion callback, but that's also a very ugly design IMHO, considering atm there's nothing at all in the media::Renderer interface that deals with media tracks. So tbh I got stuck a bit choosing which option was less ugly. Option #2 seems uglier from the design point of view, but IMHO would make the code easier to understand and a bit more robust. Option #1 would make it harder to follow the code flow for track status changes, but is less invasive for the media::Renderer interface. If you have other ideas, feel free to share, I think there should be a better solution, I just didn't have much time to think about it.
Thanks for the update, sheriff today but will dive into your comment tomorrow.
On 2017/06/14 at 19:21:49, DaleCurtis wrote: > Thanks for the update, sheriff today but will dive into your comment tomorrow. Argh, I had a long draft comment here that got deleted by BeyondCorp :( Here's take 2: I think the root of all our issues stem from the fact that we allowed MediaResource::SetStreamStatusChangedCB to be called from the Renderer; we should not have. By doing so we've introduced a back channel which has renderer state implications that the pipeline is unaware of. Instead PipelineImpl and PipelineController should have a new kTrackChanging state; they should pass a PipelineStatusCB into OnEnabledAudioTracksChanged() and OnSelectedVideoTrackChanged(). PipelineController should have queuing code for handling pending seek / pending track change operations. PipelineImpl should trigger the demuxer track change and provide a PipelineStatusCB. When that completes it should trigger a new Renderer::OnAudioTracksChanged(PipelineStatusCB), Renderer::OnVideoTracksChanged(PipelineStatusCB) methods. These methods should be split apart from the existing RendererImpl::OnStreamStatusChanged() method, but largely do as they do today. When the renderers have been flushed and restarted, they should fire the pipeline status cb. This will percolate back up and the Pipeline will resume allowing other state change events. We don't need the DemuxerStream pointer passed back to OnStreamStatusChanged() today since we only allow one enabled track, we can just reuse MediaResource::GetFirstStream(); later we could add a MediaResource::GetStreamForTrackID() to handle the multiple audio/video case. This allows us to delete the lock hack added to RendererImpl since PipelineImpl will handle media time caching during track changes (like it does for seek). It allows us not to worry about the interplay of seeks and track changes since we'll have one top level arbiter. This keeps our code consistent with what we're doing for seeks; i.e. all async operations must be complete before other state change events are allowed. Another potential improvement after the above is done s to allow renderer re-initialization similar to how config changes work today. Pipeline would call something like Renderer::Flush(<tracks>), Demuxer::OnXXXTracksChanged(), then Renderer::Initialize(<tracks>). This allows us to avoid having any special logic in RendererImpl for track changes.
On 2017/06/15 20:08:31, DaleCurtis wrote: > On 2017/06/14 at 19:21:49, DaleCurtis wrote: > > Thanks for the update, sheriff today but will dive into your comment tomorrow. > > Argh, I had a long draft comment here that got deleted by BeyondCorp :( Here's > take 2: > > I think the root of all our issues stem from the fact that we allowed > MediaResource::SetStreamStatusChangedCB to be called from the Renderer; we > should not have. By doing so we've introduced a back channel which has renderer > state implications that the pipeline is unaware of. > > Instead PipelineImpl and PipelineController should have a new kTrackChanging > state; they should pass a PipelineStatusCB into OnEnabledAudioTracksChanged() > and OnSelectedVideoTrackChanged(). PipelineController should have queuing code > for handling pending seek / pending track change operations. PipelineImpl should > trigger the demuxer track change and provide a PipelineStatusCB. When that > completes it should trigger a new > Renderer::OnAudioTracksChanged(PipelineStatusCB), > Renderer::OnVideoTracksChanged(PipelineStatusCB) methods. These methods should > be split apart from the existing RendererImpl::OnStreamStatusChanged() method, > but largely do as they do today. When the renderers have been flushed and > restarted, they should fire the pipeline status cb. This will percolate back up > and the Pipeline will resume allowing other state change events. > > We don't need the DemuxerStream pointer passed back to OnStreamStatusChanged() > today since we only allow one enabled track, we can just reuse > MediaResource::GetFirstStream(); later we could add a > MediaResource::GetStreamForTrackID() to handle the multiple audio/video case. > This allows us to delete the lock hack added to RendererImpl since PipelineImpl > will handle media time caching during track changes (like it does for seek). It > allows us not to worry about the interplay of seeks and track changes since > we'll have one top level arbiter. This keeps our code consistent with what we're > doing for seeks; i.e. all async operations must be complete before other state > change events are allowed. > > Another potential improvement after the above is done s to allow renderer > re-initialization similar to how config changes work today. Pipeline would call > something like Renderer::Flush(<tracks>), Demuxer::OnXXXTracksChanged(), then > Renderer::Initialize(<tracks>). This allows us to avoid having any special logic > in RendererImpl for track changes. Well, I suppose we could do something like that. Although it will be a much larger change than this one, as we'd have to change some core logic and APIs between Pipeline - Demuxer - Renderer. Also with the approach you suggested (where Pipeline notifies the demuxer and then the renderer about track changes) we might need to keep track of track statuses in both demuxers and renderers. Perhaps it would be better to just always handle track changes through Pipeline -> Renderer and then let the Renderer disable/enable demuxer streams as necessary? Anyway I think I'll give this approach a try later this week and see how it might work in practice.
On 2017/06/20 at 01:44:59, servolk wrote: > On 2017/06/15 20:08:31, DaleCurtis wrote: > > On 2017/06/14 at 19:21:49, DaleCurtis wrote: > > > Thanks for the update, sheriff today but will dive into your comment tomorrow. > > > > Argh, I had a long draft comment here that got deleted by BeyondCorp :( Here's > > take 2: > > > > I think the root of all our issues stem from the fact that we allowed > > MediaResource::SetStreamStatusChangedCB to be called from the Renderer; we > > should not have. By doing so we've introduced a back channel which has renderer > > state implications that the pipeline is unaware of. > > > > Instead PipelineImpl and PipelineController should have a new kTrackChanging > > state; they should pass a PipelineStatusCB into OnEnabledAudioTracksChanged() > > and OnSelectedVideoTrackChanged(). PipelineController should have queuing code > > for handling pending seek / pending track change operations. PipelineImpl should > > trigger the demuxer track change and provide a PipelineStatusCB. When that > > completes it should trigger a new > > Renderer::OnAudioTracksChanged(PipelineStatusCB), > > Renderer::OnVideoTracksChanged(PipelineStatusCB) methods. These methods should > > be split apart from the existing RendererImpl::OnStreamStatusChanged() method, > > but largely do as they do today. When the renderers have been flushed and > > restarted, they should fire the pipeline status cb. This will percolate back up > > and the Pipeline will resume allowing other state change events. > > > > We don't need the DemuxerStream pointer passed back to OnStreamStatusChanged() > > today since we only allow one enabled track, we can just reuse > > MediaResource::GetFirstStream(); later we could add a > > MediaResource::GetStreamForTrackID() to handle the multiple audio/video case. > > This allows us to delete the lock hack added to RendererImpl since PipelineImpl > > will handle media time caching during track changes (like it does for seek). It > > allows us not to worry about the interplay of seeks and track changes since > > we'll have one top level arbiter. This keeps our code consistent with what we're > > doing for seeks; i.e. all async operations must be complete before other state > > change events are allowed. > > > > Another potential improvement after the above is done s to allow renderer > > re-initialization similar to how config changes work today. Pipeline would call > > something like Renderer::Flush(<tracks>), Demuxer::OnXXXTracksChanged(), then > > Renderer::Initialize(<tracks>). This allows us to avoid having any special logic > > in RendererImpl for track changes. > > Well, I suppose we could do something like that. Although it will be a much larger change than this one, as we'd have to change some core logic and APIs between Pipeline - Demuxer - Renderer. Also with the approach you suggested (where Pipeline notifies the demuxer and then the renderer about track changes) we might need to keep track of track statuses in both demuxers and renderers. Perhaps it would be better to just always handle track changes through Pipeline -> Renderer and then let the Renderer disable/enable demuxer streams as necessary? Anyway I think I'll give this approach a try later this week and see how it might work in practice. Only pipeline should have to track the status, just like seeks do today. The pipeline prevents all reentrancy during pending operations. Demuxer and renderer can just assume that they will never have concurrent operations inflicted upon them. I.e. seek and track changes are mutually exclusive and pending seeks/track changes are queued. I think without this larger change, we're going to forever be hunting down subtle bugs of operational reentrancy. Especially in something that will end up as frequently used as seeks on tab changes; such as this CL proposes. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
