Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2549263002:
FFmpegDemuxer should fall back to disabled streams for seeking (Closed)
DescriptionFFmpegDemuxer should fall back to disabled streams for seeking
Current FFmpegDemuxer::FindPreferredStreamForSeeking logic avoids using
disabled streams. But sometimes we simply do not have any enabled
streams to choose from (e.g. there is only one video stream and it got
disabled due to tab being put into background, see crbug.com/671197).
For those cases we should resort to using disabled streams for seeking.
BUG=671197
Committed: https://crrev.com/7594a6ffe6c1eb87e377df6bf2c1e372ab7e03cf
Cr-Commit-Position: refs/heads/master@{#436838}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Revise/refactor stream selection logic #
Total comments: 9
Patch Set 3 : Rename FindStreamWithLowestStartPts -> FindStreamWithLowestStartTimestamp #Patch Set 4 : Set start time to 0 for streams with unknown start time #
Messages
Total messages: 42 (20 generated)
Description was changed from ========== FFmpegDemuxer should fall back to disabled streams for seeking Current FFmpegDemuxer::FindPreferredStreamForSeeking logic avoids using disabled streams. But sometimes we simply do not have any enabled streams to choose from (e.g. there is only one video stream and it got disabled due to tab being put into background, see crbug.com/671197). For those cases we should resort to using disabled streams for seeking. BUG=671197 ========== to ========== FFmpegDemuxer should fall back to disabled streams for seeking Current FFmpegDemuxer::FindPreferredStreamForSeeking logic avoids using disabled streams. But sometimes we simply do not have any enabled streams to choose from (e.g. there is only one video stream and it got disabled due to tab being put into background, see crbug.com/671197). For those cases we should resort to using disabled streams for seeking. BUG=671197 ==========
servolk@chromium.org changed reviewers: + avayvod@chromium.org, dalecurtis@chromium.org
On 2016/12/05 22:00:54, servolk wrote: Before this change I could reproduce the DCHECK at https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1480... fairly consistently when opening http://kwejk.pl/strona/31560 (URL taken from crash reports in crbug.com/671197) in a background tab (with --enable-features=BackgroundVideoTrackOptimization flag). After this change the DCHECK is not hit anymore.
https://codereview.chromium.org/2549263002/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2549263002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1526: FFmpegDemuxerStream* FFmpegDemuxer::FindPreferredStreamForSeeking( Does this code really need to care about disabled streams? I think it might be simpler to just remove all the enabled/disabled stream code from here.
On 2016/12/05 22:03:45, servolk wrote: > On 2016/12/05 22:00:54, servolk wrote: > > Before this change I could reproduce the DCHECK at > https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1480... > fairly consistently when opening http://kwejk.pl/strona/31560 (URL taken from > crash reports in crbug.com/671197) in a background tab (with > --enable-features=BackgroundVideoTrackOptimization flag). After this change the > DCHECK is not hit anymore. Follow up: this CL does fix the DCHECK, but another issue remains. Looks like this is a fairly common pattern in various webpages to have a <video> element that is playing some video with the loop attribute. When such a page gets into background and its video track gets disabled, then media playback ends immediately, but then gets restarted due to the loop attribute on the HTML5 tag. So we get an endless loop of <start playback>-<end playback> events, which still consumes quite a bit CPU/battery. I think we'll need to find a better solution for this. Perhaps we could avoid restarting video-only looped media elements in background tabs? I'll open a separate bug to track that issue.
https://codereview.chromium.org/2549263002/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2549263002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1526: FFmpegDemuxerStream* FFmpegDemuxer::FindPreferredStreamForSeeking( On 2016/12/05 22:18:08, DaleCurtis wrote: > Does this code really need to care about disabled streams? I think it might be > simpler to just remove all the enabled/disabled stream code from here. I think we do need to care about disabled streams here. FFmpeg demuxer has only a single read position across all streams, so ideally we want to select the enabled/selected video stream stream for seeking. If there are multiple video streams in a given file/URL and we use one of the disabled video streams for seeking, then seeking might be imprecise for the enabled video stream (e.g. it might miss the nearest keyframe where we could restart playback after seeking, and instead we might need to drop data until the next key frame arrives for the enabled video stream).
On 2016/12/05 22:19:34, servolk wrote: > On 2016/12/05 22:03:45, servolk wrote: > > On 2016/12/05 22:00:54, servolk wrote: > > > > Before this change I could reproduce the DCHECK at > > > https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1480... > > fairly consistently when opening http://kwejk.pl/strona/31560 (URL taken from > > crash reports in crbug.com/671197) in a background tab (with > > --enable-features=BackgroundVideoTrackOptimization flag). After this change > the > > DCHECK is not hit anymore. > > Follow up: this CL does fix the DCHECK, but another issue remains. Looks like > this is a fairly common pattern in various webpages to have a <video> element > that is playing some video with the loop attribute. When such a page gets into > background and its video track gets disabled, then media playback ends > immediately, but then gets restarted due to the loop attribute on the HTML5 tag. > So we get an endless loop of <start playback>-<end playback> events, which still > consumes quite a bit CPU/battery. I think we'll need to find a better solution > for this. Perhaps we could avoid restarting video-only looped media elements in > background tabs? I'll open a separate bug to track that issue. Opened crbug.com/671381 to track the issue described above.
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/2549263002/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2549263002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1569: // If there's no enabled audio/video streams, then just fall back to any Seems like instead this code should loop the whole process again with ignore_disabled_streams or something instead of trying to add a new possible path.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2549263002/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2549263002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:1569: // If there's no enabled audio/video streams, then just fall back to any On 2016/12/05 23:11:27, DaleCurtis wrote: > Seems like instead this code should loop the whole process again with > ignore_disabled_streams or something instead of trying to add a new possible > path. I agree that it would be nice and more consistent to have similar code structure for handling the two cases (enabled and non-enabled streams), but on the other hand those two cases are meaningfully different so repeating the same steps while disregarding the |enabled| status is not guaranteed to give us the best result. Upon thinking about this some more I believe the best course of action is to: 1. If there's an enabled video stream with start_pts <= seek target, then we should always prefer that (since only video streams have key frames and thus are more sensitive to the actual seek position, and there can be only one enabled video stream per HTML5MediaElement). 2. If step #1 didn't yield the result, then we should try to find the stream with lowest start pts among enabled audio/video streams (this is essentially what lines 1542-1557 are currently doing). 3. But then, if step #2 didn't yield a result, we should repeat only step #2 again while disregarding the enabled status. I.e. we should try again to find a stream with the lowest start pts this time among the disabled streams. IMHO there's no point in repeating step #1 if there's no enabled video stream, since disabled video streams may be read from, but that data will be discarded and so we don't need to be concerned about key frames for disabled video streams. I'll do a bit more refactoring in the next patchset to extract the logic for finding stream with lowest start pts among enabled/disabled streams.
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...
Thanks for tracking this! lgtm from me!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL at patchset #2, I believe it's the right fix. https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1566: // Fall back to any enabled stream, even if it's start time is unknown. TBH I'm not 100% sure about this. Should we prefer an enabled stream with an unknown (kNoTimestamp) start time over a disabled stream with a known start time? I believe we should always prefer enabled streams whenever possible and having this logic allows us to exactly preserve the old behavior tested by FFmpegDemuxerTest::Seeking_PreferredStreamSelection. But if e.g. FFmpeg handles seeking better for streams with known start times, then we can reconsider this.
https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1566: // Fall back to any enabled stream, even if it's start time is unknown. On 2016/12/06 at 18:14:17, servolk wrote: > TBH I'm not 100% sure about this. Should we prefer an enabled stream with an unknown (kNoTimestamp) start time over a disabled stream with a known start time? I believe we should always prefer enabled streams whenever possible and having this logic allows us to exactly preserve the old behavior tested by FFmpegDemuxerTest::Seeking_PreferredStreamSelection. But if e.g. FFmpeg handles seeking better for streams with known start times, then we can reconsider this. We shouldn't have any streams with a start time of kNoTimestamp. I think that's a bug. Today we make the overall start time 0 if no stream has a true start timestamp. I wonder if we should propagate 0 to all streams with an unknown start_time_. From a cursory look the only code that cares if it's NoTimestamp is this code. https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.h:333: FFmpegDemuxerStream* FindStreamWithLowestStartPts(bool enabled); StartTimestamp s/enabled/ignore_disabled_streams/ ?
https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1566: // Fall back to any enabled stream, even if it's start time is unknown. On 2016/12/06 20:18:03, DaleCurtis wrote: > On 2016/12/06 at 18:14:17, servolk wrote: > > TBH I'm not 100% sure about this. Should we prefer an enabled stream with an > unknown (kNoTimestamp) start time over a disabled stream with a known start > time? I believe we should always prefer enabled streams whenever possible and > having this logic allows us to exactly preserve the old behavior tested by > FFmpegDemuxerTest::Seeking_PreferredStreamSelection. But if e.g. FFmpeg handles > seeking better for streams with known start times, then we can reconsider this. > > We shouldn't have any streams with a start time of kNoTimestamp. I think that's > a bug. Today we make the overall start time 0 if no stream has a true start > timestamp. I wonder if we should propagate 0 to all streams with an unknown > start_time_. From a cursory look the only code that cares if it's NoTimestamp is > this code. Are you sure about this? Looking at comment https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481... and ExtractStartTime function at https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481... it seems to me like it is possible to have streams with unknown start times (kNoTimestamp and not a 0 pts). IIUC if some stream is muxed in such a way that no single packet from it is read during avformat_find_stream_info, we won't know the start time for it. When I was refactoring the seeking stream selection in https://codereview.chromium.org/2281843002 I was trying to carefully preserve the original logic that we had around handling streams with no start times.
https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.h:333: FFmpegDemuxerStream* FindStreamWithLowestStartPts(bool enabled); Done: FindStreamWithLowestStartPts -> FindStreamWithLowestStartTimestamp Re. ignore_disabled_streams - that would be actually misleading, since currently FindStreamWithLowestStartTimestamp considers only streams that are enabled or disabled, i.e. streams with enabled() matching the enabled parameter, so currently calling FindStreamWithLowestStartTimestamp(false) would return the stream with lowest start time among disabled streams only. If the input parameter was names ignore_disabled_streams then I'd expected FindStreamWithLowestStartTimestamp(false) to return the stream with lowest start time among enabled and disabled streams.
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/2549263002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1566: // Fall back to any enabled stream, even if it's start time is unknown. On 2016/12/06 at 22:05:29, servolk wrote: > On 2016/12/06 20:18:03, DaleCurtis wrote: > > On 2016/12/06 at 18:14:17, servolk wrote: > > > TBH I'm not 100% sure about this. Should we prefer an enabled stream with an > > unknown (kNoTimestamp) start time over a disabled stream with a known start > > time? I believe we should always prefer enabled streams whenever possible and > > having this logic allows us to exactly preserve the old behavior tested by > > FFmpegDemuxerTest::Seeking_PreferredStreamSelection. But if e.g. FFmpeg handles > > seeking better for streams with known start times, then we can reconsider this. > > > > We shouldn't have any streams with a start time of kNoTimestamp. I think that's > > a bug. Today we make the overall start time 0 if no stream has a true start > > timestamp. I wonder if we should propagate 0 to all streams with an unknown > > start_time_. From a cursory look the only code that cares if it's NoTimestamp is > > this code. > > Are you sure about this? Looking at comment https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481... and ExtractStartTime function at https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481... it seems to me like it is possible to have streams with unknown start times (kNoTimestamp and not a 0 pts). > IIUC if some stream is muxed in such a way that no single packet from it is read during avformat_find_stream_info, we won't know the start time for it. > When I was refactoring the seeking stream selection in https://codereview.chromium.org/2281843002 I was trying to carefully preserve the original logic that we had around handling streams with no start times. We never correct the start time if this is true, so the stream forever lingers with kNoTimestamp and the demuxer always marks the start time as zero in this case.
https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1566: // Fall back to any enabled stream, even if it's start time is unknown. On 2016/12/06 22:17:09, DaleCurtis wrote: > On 2016/12/06 at 22:05:29, servolk wrote: > > On 2016/12/06 20:18:03, DaleCurtis wrote: > > > On 2016/12/06 at 18:14:17, servolk wrote: > > > > TBH I'm not 100% sure about this. Should we prefer an enabled stream with > an > > > unknown (kNoTimestamp) start time over a disabled stream with a known start > > > time? I believe we should always prefer enabled streams whenever possible > and > > > having this logic allows us to exactly preserve the old behavior tested by > > > FFmpegDemuxerTest::Seeking_PreferredStreamSelection. But if e.g. FFmpeg > handles > > > seeking better for streams with known start times, then we can reconsider > this. > > > > > > We shouldn't have any streams with a start time of kNoTimestamp. I think > that's > > > a bug. Today we make the overall start time 0 if no stream has a true start > > > timestamp. I wonder if we should propagate 0 to all streams with an unknown > > > start_time_. From a cursory look the only code that cares if it's > NoTimestamp is > > > this code. > > > > Are you sure about this? Looking at comment > https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481... > and ExtractStartTime function at > https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481... > it seems to me like it is possible to have streams with unknown start times > (kNoTimestamp and not a 0 pts). > > IIUC if some stream is muxed in such a way that no single packet from it is > read during avformat_find_stream_info, we won't know the start time for it. > > When I was refactoring the seeking stream selection in > https://codereview.chromium.org/2281843002 I was trying to carefully preserve > the original logic that we had around handling streams with no start times. > > We never correct the start time if this is true, so the stream forever lingers > with kNoTimestamp and the demuxer always marks the start time as zero in this > case. Ok, so in other words this means that it's the right choice here to prefer enabled streams with an unknown / zero start time for seeking over disabled streams with known start time, right? Or are you saying that we should explicitly set start time to 0 for demuxer streams whose start time could not be determined reliably in FFmpegDemuxer::OnFindStreamInfoDone and then just DCHECK here and elsewhere that stream start time is never kNoTimestamp?
https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1566: // Fall back to any enabled stream, even if it's start time is unknown. On 2016/12/06 at 23:00:41, servolk wrote: > On 2016/12/06 22:17:09, DaleCurtis wrote: > > On 2016/12/06 at 22:05:29, servolk wrote: > > > On 2016/12/06 20:18:03, DaleCurtis wrote: > > > > On 2016/12/06 at 18:14:17, servolk wrote: > > > > > TBH I'm not 100% sure about this. Should we prefer an enabled stream with > > an > > > > unknown (kNoTimestamp) start time over a disabled stream with a known start > > > > time? I believe we should always prefer enabled streams whenever possible > > and > > > > having this logic allows us to exactly preserve the old behavior tested by > > > > FFmpegDemuxerTest::Seeking_PreferredStreamSelection. But if e.g. FFmpeg > > handles > > > > seeking better for streams with known start times, then we can reconsider > > this. > > > > > > > > We shouldn't have any streams with a start time of kNoTimestamp. I think > > that's > > > > a bug. Today we make the overall start time 0 if no stream has a true start > > > > timestamp. I wonder if we should propagate 0 to all streams with an unknown > > > > start_time_. From a cursory look the only code that cares if it's > > NoTimestamp is > > > > this code. > > > > > > Are you sure about this? Looking at comment > > https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481... > > and ExtractStartTime function at > > https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481... > > it seems to me like it is possible to have streams with unknown start times > > (kNoTimestamp and not a 0 pts). > > > IIUC if some stream is muxed in such a way that no single packet from it is > > read during avformat_find_stream_info, we won't know the start time for it. > > > When I was refactoring the seeking stream selection in > > https://codereview.chromium.org/2281843002 I was trying to carefully preserve > > the original logic that we had around handling streams with no start times. > > > > We never correct the start time if this is true, so the stream forever lingers > > with kNoTimestamp and the demuxer always marks the start time as zero in this > > case. > > Ok, so in other words this means that it's the right choice here to prefer enabled streams with an unknown / zero start time for seeking over disabled streams with known start time, right? > Or are you saying that we should explicitly set start time to 0 for demuxer streams whose start time could not be determined reliably in FFmpegDemuxer::OnFindStreamInfoDone and then just DCHECK here and elsewhere that stream start time is never kNoTimestamp? I think we can set the start_time to zero when it's unknown. Do you see any instances other than this code that cares about the kNoTimestamp value for start_time?
https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:1566: // Fall back to any enabled stream, even if it's start time is unknown. On 2016/12/06 23:29:05, DaleCurtis wrote: > On 2016/12/06 at 23:00:41, servolk wrote: > > On 2016/12/06 22:17:09, DaleCurtis wrote: > > > On 2016/12/06 at 22:05:29, servolk wrote: > > > > On 2016/12/06 20:18:03, DaleCurtis wrote: > > > > > On 2016/12/06 at 18:14:17, servolk wrote: > > > > > > TBH I'm not 100% sure about this. Should we prefer an enabled stream > with > > > an > > > > > unknown (kNoTimestamp) start time over a disabled stream with a known > start > > > > > time? I believe we should always prefer enabled streams whenever > possible > > > and > > > > > having this logic allows us to exactly preserve the old behavior tested > by > > > > > FFmpegDemuxerTest::Seeking_PreferredStreamSelection. But if e.g. FFmpeg > > > handles > > > > > seeking better for streams with known start times, then we can > reconsider > > > this. > > > > > > > > > > We shouldn't have any streams with a start time of kNoTimestamp. I think > > > that's > > > > > a bug. Today we make the overall start time 0 if no stream has a true > start > > > > > timestamp. I wonder if we should propagate 0 to all streams with an > unknown > > > > > start_time_. From a cursory look the only code that cares if it's > > > NoTimestamp is > > > > > this code. > > > > > > > > Are you sure about this? Looking at comment > > > > https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481... > > > and ExtractStartTime function at > > > > https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481... > > > it seems to me like it is possible to have streams with unknown start times > > > (kNoTimestamp and not a 0 pts). > > > > IIUC if some stream is muxed in such a way that no single packet from it > is > > > read during avformat_find_stream_info, we won't know the start time for it. > > > > When I was refactoring the seeking stream selection in > > > https://codereview.chromium.org/2281843002 I was trying to carefully > preserve > > > the original logic that we had around handling streams with no start times. > > > > > > We never correct the start time if this is true, so the stream forever > lingers > > > with kNoTimestamp and the demuxer always marks the start time as zero in > this > > > case. > > > > Ok, so in other words this means that it's the right choice here to prefer > enabled streams with an unknown / zero start time for seeking over disabled > streams with known start time, right? > > Or are you saying that we should explicitly set start time to 0 for demuxer > streams whose start time could not be determined reliably in > FFmpegDemuxer::OnFindStreamInfoDone and then just DCHECK here and elsewhere that > stream start time is never kNoTimestamp? > > I think we can set the start_time to zero when it's unknown. Do you see any > instances other than this code that cares about the kNoTimestamp value for > start_time? You are right, looks like FFmpegDemuxerStream::start_time() is only used here, so we can experiment with using 0 instead of kNoTimestamp in ExtractStartTime and it shouldn't affect anything else, I'll give it a try.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/06 23:43:34, servolk wrote: > https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... > File media/filters/ffmpeg_demuxer.cc (right): > > https://codereview.chromium.org/2549263002/diff/20001/media/filters/ffmpeg_de... > media/filters/ffmpeg_demuxer.cc:1566: // Fall back to any enabled stream, even > if it's start time is unknown. > On 2016/12/06 23:29:05, DaleCurtis wrote: > > On 2016/12/06 at 23:00:41, servolk wrote: > > > On 2016/12/06 22:17:09, DaleCurtis wrote: > > > > On 2016/12/06 at 22:05:29, servolk wrote: > > > > > On 2016/12/06 20:18:03, DaleCurtis wrote: > > > > > > On 2016/12/06 at 18:14:17, servolk wrote: > > > > > > > TBH I'm not 100% sure about this. Should we prefer an enabled stream > > with > > > > an > > > > > > unknown (kNoTimestamp) start time over a disabled stream with a known > > start > > > > > > time? I believe we should always prefer enabled streams whenever > > possible > > > > and > > > > > > having this logic allows us to exactly preserve the old behavior > tested > > by > > > > > > FFmpegDemuxerTest::Seeking_PreferredStreamSelection. But if e.g. > FFmpeg > > > > handles > > > > > > seeking better for streams with known start times, then we can > > reconsider > > > > this. > > > > > > > > > > > > We shouldn't have any streams with a start time of kNoTimestamp. I > think > > > > that's > > > > > > a bug. Today we make the overall start time 0 if no stream has a true > > start > > > > > > timestamp. I wonder if we should propagate 0 to all streams with an > > unknown > > > > > > start_time_. From a cursory look the only code that cares if it's > > > > NoTimestamp is > > > > > > this code. > > > > > > > > > > Are you sure about this? Looking at comment > > > > > > > https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481... > > > > and ExtractStartTime function at > > > > > > > https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481... > > > > it seems to me like it is possible to have streams with unknown start > times > > > > (kNoTimestamp and not a 0 pts). > > > > > IIUC if some stream is muxed in such a way that no single packet from it > > is > > > > read during avformat_find_stream_info, we won't know the start time for > it. > > > > > When I was refactoring the seeking stream selection in > > > > https://codereview.chromium.org/2281843002 I was trying to carefully > > preserve > > > > the original logic that we had around handling streams with no start > times. > > > > > > > > We never correct the start time if this is true, so the stream forever > > lingers > > > > with kNoTimestamp and the demuxer always marks the start time as zero in > > this > > > > case. > > > > > > Ok, so in other words this means that it's the right choice here to prefer > > enabled streams with an unknown / zero start time for seeking over disabled > > streams with known start time, right? > > > Or are you saying that we should explicitly set start time to 0 for demuxer > > streams whose start time could not be determined reliably in > > FFmpegDemuxer::OnFindStreamInfoDone and then just DCHECK here and elsewhere > that > > stream start time is never kNoTimestamp? > > > > I think we can set the start_time to zero when it's unknown. Do you see any > > instances other than this code that cares about the kNoTimestamp value for > > start_time? > > You are right, looks like FFmpegDemuxerStream::start_time() is only used here, > so we can experiment with using 0 instead of kNoTimestamp in ExtractStartTime > and it shouldn't affect anything else, I'll give it a try. Ok, in the latest patchset I've made a change to set start_time to 0 for stream which had kNoTimestamp start_time in the past, PTAL.
lgtm
The CQ bit was unchecked by servolk@chromium.org
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2549263002/#ps60001 (title: "Set start time to 0 for streams with unknown start time")
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": 1481072479733850,
"parent_rev": "d565a77f05e5903efdd23f806b4b10f7b7c5fb13", "commit_rev":
"1c57ebf923a24875e405f25c5d8b8c7753357d07"}
Message was sent while issue was closed.
Description was changed from ========== FFmpegDemuxer should fall back to disabled streams for seeking Current FFmpegDemuxer::FindPreferredStreamForSeeking logic avoids using disabled streams. But sometimes we simply do not have any enabled streams to choose from (e.g. there is only one video stream and it got disabled due to tab being put into background, see crbug.com/671197). For those cases we should resort to using disabled streams for seeking. BUG=671197 ========== to ========== FFmpegDemuxer should fall back to disabled streams for seeking Current FFmpegDemuxer::FindPreferredStreamForSeeking logic avoids using disabled streams. But sometimes we simply do not have any enabled streams to choose from (e.g. there is only one video stream and it got disabled due to tab being put into background, see crbug.com/671197). For those cases we should resort to using disabled streams for seeking. BUG=671197 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== FFmpegDemuxer should fall back to disabled streams for seeking Current FFmpegDemuxer::FindPreferredStreamForSeeking logic avoids using disabled streams. But sometimes we simply do not have any enabled streams to choose from (e.g. there is only one video stream and it got disabled due to tab being put into background, see crbug.com/671197). For those cases we should resort to using disabled streams for seeking. BUG=671197 ========== to ========== FFmpegDemuxer should fall back to disabled streams for seeking Current FFmpegDemuxer::FindPreferredStreamForSeeking logic avoids using disabled streams. But sometimes we simply do not have any enabled streams to choose from (e.g. there is only one video stream and it got disabled due to tab being put into background, see crbug.com/671197). For those cases we should resort to using disabled streams for seeking. BUG=671197 Committed: https://crrev.com/7594a6ffe6c1eb87e377df6bf2c1e372ab7e03cf Cr-Commit-Position: refs/heads/master@{#436838} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7594a6ffe6c1eb87e377df6bf2c1e372ab7e03cf Cr-Commit-Position: refs/heads/master@{#436838} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
