Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1349973002:
Fix seeking back in the new MSE GC algorithm (Closed)
DescriptionFix seeking back in the new MSE GC algorithm
BUG=532136
Patch Set 1 #
Total comments: 10
Patch Set 2 : CR feedback #
Messages
Total messages: 6 (1 generated)
wolenetz@chromium.org changed reviewers: + wolenetz@chromium.org
Some nits, but as discussed in chat, this is likely to become the next patch set (6?) of https://codereview.chromium.org/1347483003/, and higher-level comments were the focus of this current review iteration: https://codereview.chromium.org/1349973002/diff/1/media/filters/chunk_demuxer... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1349973002/diff/1/media/filters/chunk_demuxer... media/filters/chunk_demuxer_unittest.cc:3415: // GC should be able to evict frames in the currently buffered range, since nit:s/range/ranges/ https://codereview.chromium.org/1349973002/diff/1/media/filters/chunk_demuxer... media/filters/chunk_demuxer_unittest.cc:3434: // GC should be able to evict frames from the front, since nit: it's collecting from front conservatively, then greedily. last append was 2115, so greedy was needed to get enough room. comment is unclear about this. Regarding test coverage, just one more I think would be good: GCDuringSeek_MultipleRanges_SeekInbetween2, where last append was the first of the two ranges in timeline. This would need to exercise and verify collect_from_back_until_last_appended_GOP. https://codereview.chromium.org/1349973002/diff/1/media/filters/chunk_demuxer... media/filters/chunk_demuxer_unittest.cc:3455: base::TimeDelta seek_time = base::TimeDelta::FromMilliseconds(0); weak nit: here and elsewhere, just base::TimeDelta() for a time of 0. Existing unit test code here is inconsistent, hence the weakness of this nit... https://codereview.chromium.org/1349973002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1349973002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream.cc:623: LOG(WARNING) << "GarbageCollectIfNeeded: memory_limit_=" << memory_limit_ Please change to LIMITED_MEDIA_LOG(DEBUG...), so chrome://media-internals can get detail on at least desktop builds when this path is reached. I'll save other nit-like comments for later. https://codereview.chromium.org/1349973002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream.cc:639: for (auto& r : ranges_) { Bear with me. I think the tl;dr is this is ok. I wonder if we're possibly being too greedy: this logic could remove the keyframe upon which media_time depends. It is a hard problem to determine if this is the case, though, since MSE spec allows (and Chrome will soon) media segments to begin with a non-keyframe. |media_time| could be within one of these dependent media segments that follows an existing buffered media range that contains the prerequisite keyframe data. To be safest, if there is a buffered range prior to |media_time|, then it must contain a keyframe. Maybe we should not collect the last keyframe (nor its dependent frames) from that buffered range, in case the eventually-appended-media at |media_time| depends upon it. Granted, I think this level of protection may be also going too far, since the MSE coded frame processing algorithm drops non-keyframes following a discontinuity, and such a discontinuity would occur if appends go out of order. So, last append would need to be < media_time for CFP to allow newly appended non-keyframes to possibly become buffered following the last append. Soooooo.... For now, I think this logic seems ok. A couple other comments: Is there a reason to not initialize seek_pending (local) to whatever the current value of |seek_pending_| (member) is prior to this loop? IIUC, media_time (when seeking) is a secondary indicator: seek_pending_ should be the true knowledge within the SBS of whether or not there is currently an unsatisfied seek. How might media_time have more "correct" indication? Does seek_buffer_timestamp_ differ from media_time when there's an unsatisfied seek and GC is called? Finally: If we need to find if there is a potential range for which media_time is a member, we might be able to use https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/sour... although this method allows for returning true when |media_time_| is "next in sequence" to the end of the matched range, so I don't think using that helper as-is would be viable. Is there not some other helper that does this loop already? (and if not, perhaps making this loop a helper or modifying the existing helper to conditionally be more strict, might be a better way to reuse existing code).
https://codereview.chromium.org/1349973002/diff/1/media/filters/chunk_demuxer... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1349973002/diff/1/media/filters/chunk_demuxer... media/filters/chunk_demuxer_unittest.cc:3415: // GC should be able to evict frames in the currently buffered range, since On 2015/09/17 18:57:54, wolenetz wrote: > nit:s/range/ranges/ Done. https://codereview.chromium.org/1349973002/diff/1/media/filters/chunk_demuxer... media/filters/chunk_demuxer_unittest.cc:3434: // GC should be able to evict frames from the front, since On 2015/09/17 18:57:54, wolenetz wrote: > nit: it's collecting from front conservatively, then greedily. last append was > 2115, so greedy was needed to get enough room. comment is unclear about this. > > Regarding test coverage, just one more I think would be good: > GCDuringSeek_MultipleRanges_SeekInbetween2, where last append was the first of > the two ranges in timeline. This would need to exercise and verify > collect_from_back_until_last_appended_GOP. Good point, I've expanded this comment to explain better what's going on this this case. I've also added a new test case where the appended data in the first range was appended more recently than in the second range and that makes GC algorithm choose to remove the second buffered range (since the first one was appended more recently than the second). https://codereview.chromium.org/1349973002/diff/1/media/filters/chunk_demuxer... media/filters/chunk_demuxer_unittest.cc:3455: base::TimeDelta seek_time = base::TimeDelta::FromMilliseconds(0); On 2015/09/17 18:57:54, wolenetz wrote: > weak nit: here and elsewhere, just base::TimeDelta() for a time of 0. Existing > unit test code here is inconsistent, hence the weakness of this nit... It makes sense in retrospect, but I just didn't realize that TimeDelta has a default constructor that inits it to 0. I've made changes to use it for now, although perhaps in the future we could consider introducing operator TimeDelta(int ms) just in this file locally, it could make test code a bit cleaner by avoiding all those repeated FromMilliseconds calls. https://codereview.chromium.org/1349973002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1349973002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream.cc:623: LOG(WARNING) << "GarbageCollectIfNeeded: memory_limit_=" << memory_limit_ On 2015/09/17 18:57:54, wolenetz wrote: > Please change to LIMITED_MEDIA_LOG(DEBUG...), so chrome://media-internals can > get detail on at least desktop builds when this path is reached. > > I'll save other nit-like comments for later. Done. https://codereview.chromium.org/1349973002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream.cc:639: for (auto& r : ranges_) { On 2015/09/17 18:57:54, wolenetz wrote: > Bear with me. I think the tl;dr is this is ok. > > I wonder if we're possibly being too greedy: this logic could remove the > keyframe upon which media_time depends. It is a hard problem to determine if > this is the case, though, since MSE spec allows (and Chrome will soon) media > segments to begin with a non-keyframe. |media_time| could be within one of these > dependent media segments that follows an existing buffered media range that > contains the prerequisite keyframe data. > > To be safest, if there is a buffered range prior to |media_time|, then it must > contain a keyframe. Maybe we should not collect the last keyframe (nor its > dependent frames) from that buffered range, in case the > eventually-appended-media at |media_time| depends upon it. > > Granted, I think this level of protection may be also going too far, since the > MSE coded frame processing algorithm drops non-keyframes following a > discontinuity, and such a discontinuity would occur if appends go out of order. > So, last append would need to be < media_time for CFP to allow newly appended > non-keyframes to possibly become buffered following the last append. > > Soooooo.... For now, I think this logic seems ok. > > A couple other comments: > Is there a reason to not initialize seek_pending (local) to whatever the current > value of |seek_pending_| (member) is prior to this loop? > IIUC, media_time (when seeking) is a secondary indicator: seek_pending_ should > be the true knowledge within the SBS of whether or not there is currently an > unsatisfied seek. How might media_time have more "correct" indication? Does > seek_buffer_timestamp_ differ from media_time when there's an unsatisfied seek > and GC is called? > > Finally: > If we need to find if there is a potential range for which media_time is a > member, we might be able to use > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/sour... > although this method allows for returning true when |media_time_| is "next in > sequence" to the end of the matched range, so I don't think using that helper > as-is would be viable. Is there not some other helper that does this loop > already? (and if not, perhaps making this loop a helper or modifying the > existing helper to conditionally be more strict, might be a better way to reuse > existing code). Wait, as far as I understand GOP handling is already done by FreeBuffers (which uses SourceBufferRange::DeleteGOP* methods, so we always delete GOP at a time, i.e. key frame and all data that depends on it). I'm not sure how this should work in cases where newly appended segments are allowed to start with non-keyframes. I agree that for now this logic is probably ok, since we don't currently allow that. Re seek_pending - I just didn't know that SBS already keeps track of that, we can indeed just use the IsSeekPending flag instead of trying to detect pending seek by checking media_time against buffered ranges. So then we don't need to use iteration over ranges and don't need to use FindExistingRangeFor.
On 2015/09/18 01:29:28, servolk wrote: > https://codereview.chromium.org/1349973002/diff/1/media/filters/chunk_demuxer... > File media/filters/chunk_demuxer_unittest.cc (right): > > https://codereview.chromium.org/1349973002/diff/1/media/filters/chunk_demuxer... > media/filters/chunk_demuxer_unittest.cc:3415: // GC should be able to evict > frames in the currently buffered range, since > On 2015/09/17 18:57:54, wolenetz wrote: > > nit:s/range/ranges/ > > Done. > > https://codereview.chromium.org/1349973002/diff/1/media/filters/chunk_demuxer... > media/filters/chunk_demuxer_unittest.cc:3434: // GC should be able to evict > frames from the front, since > On 2015/09/17 18:57:54, wolenetz wrote: > > nit: it's collecting from front conservatively, then greedily. last append was > > 2115, so greedy was needed to get enough room. comment is unclear about this. > > > > Regarding test coverage, just one more I think would be good: > > GCDuringSeek_MultipleRanges_SeekInbetween2, where last append was the first of > > the two ranges in timeline. This would need to exercise and verify > > collect_from_back_until_last_appended_GOP. > > Good point, I've expanded this comment to explain better what's going on this > this case. > I've also added a new test case where the appended data in the first range was > appended more recently than in the second range and that makes GC algorithm > choose to remove the second buffered range (since the first one was appended > more recently than the second). > > https://codereview.chromium.org/1349973002/diff/1/media/filters/chunk_demuxer... > media/filters/chunk_demuxer_unittest.cc:3455: base::TimeDelta seek_time = > base::TimeDelta::FromMilliseconds(0); > On 2015/09/17 18:57:54, wolenetz wrote: > > weak nit: here and elsewhere, just base::TimeDelta() for a time of 0. Existing > > unit test code here is inconsistent, hence the weakness of this nit... > > It makes sense in retrospect, but I just didn't realize that TimeDelta has a > default constructor that inits it to 0. I've made changes to use it for now, > although perhaps in the future we could consider introducing operator > TimeDelta(int ms) just in this file locally, it could make test code a bit > cleaner by avoiding all those repeated FromMilliseconds calls. > > https://codereview.chromium.org/1349973002/diff/1/media/filters/source_buffer... > File media/filters/source_buffer_stream.cc (right): > > https://codereview.chromium.org/1349973002/diff/1/media/filters/source_buffer... > media/filters/source_buffer_stream.cc:623: LOG(WARNING) << > "GarbageCollectIfNeeded: memory_limit_=" << memory_limit_ > On 2015/09/17 18:57:54, wolenetz wrote: > > Please change to LIMITED_MEDIA_LOG(DEBUG...), so chrome://media-internals can > > get detail on at least desktop builds when this path is reached. > > > > I'll save other nit-like comments for later. > > Done. > > https://codereview.chromium.org/1349973002/diff/1/media/filters/source_buffer... > media/filters/source_buffer_stream.cc:639: for (auto& r : ranges_) { > On 2015/09/17 18:57:54, wolenetz wrote: > > Bear with me. I think the tl;dr is this is ok. > > > > I wonder if we're possibly being too greedy: this logic could remove the > > keyframe upon which media_time depends. It is a hard problem to determine if > > this is the case, though, since MSE spec allows (and Chrome will soon) media > > segments to begin with a non-keyframe. |media_time| could be within one of > these > > dependent media segments that follows an existing buffered media range that > > contains the prerequisite keyframe data. > > > > To be safest, if there is a buffered range prior to |media_time|, then it must > > contain a keyframe. Maybe we should not collect the last keyframe (nor its > > dependent frames) from that buffered range, in case the > > eventually-appended-media at |media_time| depends upon it. > > > > Granted, I think this level of protection may be also going too far, since the > > MSE coded frame processing algorithm drops non-keyframes following a > > discontinuity, and such a discontinuity would occur if appends go out of > order. > > So, last append would need to be < media_time for CFP to allow newly appended > > non-keyframes to possibly become buffered following the last append. > > > > Soooooo.... For now, I think this logic seems ok. > > > > A couple other comments: > > Is there a reason to not initialize seek_pending (local) to whatever the > current > > value of |seek_pending_| (member) is prior to this loop? > > IIUC, media_time (when seeking) is a secondary indicator: seek_pending_ should > > be the true knowledge within the SBS of whether or not there is currently an > > unsatisfied seek. How might media_time have more "correct" indication? Does > > seek_buffer_timestamp_ differ from media_time when there's an unsatisfied seek > > and GC is called? > > > > Finally: > > If we need to find if there is a potential range for which media_time is a > > member, we might be able to use > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/sour... > > although this method allows for returning true when |media_time_| is "next in > > sequence" to the end of the matched range, so I don't think using that helper > > as-is would be viable. Is there not some other helper that does this loop > > already? (and if not, perhaps making this loop a helper or modifying the > > existing helper to conditionally be more strict, might be a better way to > reuse > > existing code). > > Wait, as far as I understand GOP handling is already done by FreeBuffers (which > uses SourceBufferRange::DeleteGOP* methods, so we always delete GOP at a time, > i.e. key frame and all data that depends on it). I'm not sure how this should > work in cases where newly appended segments are allowed to start with > non-keyframes. I agree that for now this logic is probably ok, since we don't > currently allow that. > Re seek_pending - I just didn't know that SBS already keeps track of that, we > can indeed just use the IsSeekPending flag instead of trying to detect pending > seek by checking media_time against buffered ranges. So then we don't need to > use iteration over ranges and don't need to use FindExistingRangeFor. Ok. I'll reply on the main CL (https://codereview.chromium.org/1347483003/), since the most recent patch set from here (PS#2) has been merged into the main CL.
Also, regarding: "Wait, as far as I understand GOP handling is already done by FreeBuffers (which uses SourceBufferRange::DeleteGOP* methods, so we always delete GOP at a time, i.e. key frame and all data that depends on it). I'm not sure how this should work in cases where newly appended segments are allowed to start with non-keyframes. I agree that for now this logic is probably ok, since we don't currently allow that." We do collect GOP-at-a-time, but we don't know at GC time if there is the next append continues a GOP that was only partially appended in the last append. This isn't a change from previous code, so addressing this situation is outside the scope of your GC-seek-fixing CL(s) like this one and the main one. Essentially, I might need to make the coded frame processor more aware of whenever the last appended GOP for a track is removed (either by GC or explicit removal), so that it has the option of also triggering discontinuity detection if the next-append for that track appears adjacent but doesn't begin with a keyframe (bug 229412's fix should include this logic or similar eventually).
On 2015/09/21 20:23:13, wolenetz wrote: > Also, regarding: > "Wait, as far as I understand GOP handling is already done by FreeBuffers > (which uses SourceBufferRange::DeleteGOP* methods, so we always delete > GOP at a time, i.e. key frame and all data that depends on it). I'm not > sure how this should work in cases where newly appended segments are > allowed to start with non-keyframes. I agree that for now this logic is > probably ok, since we don't currently allow that." > > We do collect GOP-at-a-time, but we don't know at GC time if there is the next > append continues a GOP that was only partially appended in the last append. This > isn't a change from previous code, so addressing this situation is outside the > scope of your GC-seek-fixing CL(s) like this one and the main one. > Essentially, I might need to make the coded frame processor more aware of > whenever the last appended GOP for a track is removed (either by GC or explicit > removal), so that it has the option of also triggering discontinuity detection > if the next-append for that track appears adjacent but doesn't begin with a > keyframe (bug 229412's fix should include this logic or similar eventually). Yeah, ok, got it. Indeed we don't know during GC if there is another append coming in the future that it going to complete the GOP. But you are right that that's outside of the scope of this CL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
