Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1018373003:
Improving WebM video duration estimation. (Closed)
|
Created:
5 years, 9 months ago by chcunningham Modified:
5 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, DaleCurtis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImproving WebM duration estimation.
1. Increasing hard-coded estimates to fix stalls in low fps YouTube videos
2. Moving video to use max duration to avoid stalls.
3. Adjusting estimates in SourceBufferRange upon future appends.
*More on hard-coded estimates*
The hard-coded video duration estimate was changed from 42ms (24fps) to 63ms (16fps). While 42ms is more common than 63ms, both are just wild guesses and 42ms was causing low framerate videos (12fps and lower) to stall by incorrectly triggering gaps in the MSE buffered range. 63ms will prevent these gaps for video as low as 8fps. To the extent that 63ms favors over-estimation, this is mitigated by the adjustments in SourceBufferRange.
*What about audio*
This CL lays the foundation for a similar change to audio's estimation. I'm splitting them up to keep the size reasonable.
Internal Google bug: b/18090240
Committed: https://crrev.com/985c1f983bf4092dc5504a3c363b3fe2a535c03a
Cr-Commit-Position: refs/heads/master@{#325507}
Patch Set 1 : Add duration estimate flag. Adjust estimate in SourceBufferStream for in-order appends. #Patch Set 2 : Relaxing restrictions on adjusting estimated duration. Moved to SourceBufferRange. #Patch Set 3 : Adding limited media log (10 times max) for WebM duration estimates. #
Total comments: 32
Patch Set 4 : Feedback, round 1 (no tests yet) #Patch Set 5 : Adding tests and fixing bug #
Total comments: 24
Patch Set 6 : Feedback, round 1 #Patch Set 7 : Feedback, round 2 #Patch Set 8 : Fixing try failure, remove unused variable for some builds. #
Messages
Total messages: 26 (6 generated)
chcunningham@chromium.org changed reviewers: + wolenetz@chromium.org
Generally we’ve been discussing what currently happens in cases of appending frames that overlap, what should happen as spec’d, and what might be spec’d in the future. For instance… first append buffer A: [-------------] hen buffer b (in middle of buffer A's range): [-----] What should happen to overlapped frame? Duration change? How does this affect reporting of buffered ranges? Is there anything we can learn from spec/impl that might broadly apply to estimated durations (which aren't at all part of the spec)? *The current impl* So what does SBS currently do? It does *not* remove the previously appended (about-to-be-overlapped). It doesn't currently (prior to my change) affect that frame at all. As we ::PrepareRangesForNextAppend, we have two calls to ::RemoveInternal. 1) RemoveInternal(prev_timestamp, next_timestamp, true, deleted_buffers); This one removes *after* the prev_timestamp (thats the behavior of the "true" argument). In math notation: Remove (prev_timestamp, next_timestamp] (the ] is a bug as we've discussed) 2) RemoveInternal(start, end, is_exclusive, deleted_buffers); This one removes *after* the start of the new append. Inclusive/exlusive is contextual here, so I won't bother with math notation. But its definitely definitely not going to remove the previous append. One interesting thing to remember is that if at any point in FP algorithm you remove a frame, you must remove all frames AFTER that frame which depend on the removed frame. This doesn't affect our overlap case (as our original overlapped frame is always BEFORE), but still, this is good to keep in mind. This is spec'd and implemented SBS behavior. *The spec* I don't find that the spec covers this around the frame processing algorithm. I broke down what it does say piece by piece. I looked for a case where it might be implicitly covered as part of the definition for "buffered" for SourceBuffers, but found that "buffered" is not defined verbosely enough to answer our question. We've discussed filing a spec bug to clear this up, and an perhaps an additional spec bug to standardize how to do estimation for WebM. Happy to file these... lets discuss exactly what they should say. For MSE overlap: Its easy enough to describe whats missing from the spec. Should we also give a recommendation? For video it would be easy to just scale back duration as I'm doing with estimated duration. For audio, perhaps we splice? Splice if not estimated? For WebM estimation: Again, its easy to describe the problem, and I'm hoping to just recommend the solution you see in patch set two (if we haven't overlooked anything). WDYT?
My first upload is the narrow/conservative approach. If we're appending buffers in order and we find that any is over-estimated, trim it back. After discussion with Matt, we found this misses a few cases where things are not appended in-order and perhaps the in-order check is not really important. My second upload is much more general. If at any time we append a buffer onto a SourceBufferRange, and find that the end of the range (prior to append) has an over-estimated duration, then trim it back. This works for both the merge case (where new buffers must be merged with buffers *later* in the stream), and also the serial case (where new buffers overlap those *earlier* in the stream). This came out of discussions with Matt where we ran through scenarios where media has estimated duration *Success so far* The changes uploaded here fix both the broken youtube videos (which had gaps in their buffered ranges due to hard coded estimates that were too small). From the logs, we see the estimation correction code is also working quite nicely: webm_cluster_parser.cc(793)] GetDurationEstimate : using hardcoded default duration webm_cluster_parser.cc(687)] ApplyDurationEstimateIfNeeded() : new dur : ts 0 dur 0.063 kf 1 size 48 webm_cluster_parser.cc(793)] GetDurationEstimate : using hardcoded default duration webm_cluster_parser.cc(687)] ApplyDurationEstimateIfNeeded() : new dur : ts 0.125 dur 0.063 kf 1 size 725 source_buffer_range.cc(85)] Replacing estimated duration (0.063s) from last append with derived duration (0.125s). source_buffer_range.cc(85)] Replacing estimated duration (0.063s) from last append with derived duration (0.125s). *Success for audio (later cl)* I've also prototyped substituting min for max for audio duration estimation in webm cluster parser. I added a check to disable splicing if estimated buffers are encountered. This works well to fix the issues observed by tbutter in this bug without causing extra splicing. webm_cluster_parser.cc(687)] ApplyDurationEstimateIfNeeded() : new dur : ts 9.965 dur 0.043 kf 1 size 243 webm_cluster_parser.cc(687)] ApplyDurationEstimateIfNeeded() : new dur : ts 9.96 dur 0.04 kf 0 size 1450 webm_cluster_parser.cc(687)] ApplyDurationEstimateIfNeeded() : new dur : ts 19.971 dur 0.043 kf 1 size 241 webm_cluster_parser.cc(687)] ApplyDurationEstimateIfNeeded() : new dur : ts 19.96 dur 0.04 kf 0 size 1074 source_buffer_stream.cc(1525)] Can't generate splice: pre splice buffer has estimated duration source_buffer_range.cc(85)] Replacing estimated duration (0.043s) from last append with derived duration (0.042s).
w.r.t: > One interesting thing to remember is that if at any point in FP algorithm you > remove a frame, you must remove all frames AFTER that frame which depend on the > removed frame. This doesn't affect our overlap case (as our original overlapped > frame is always BEFORE), but still, this is good to keep in mind. This is spec'd > and implemented SBS behavior. IIUC, it *should* affect our overlap case, if the "first append buffer A" were changed to "first append GOP A containing multiple dependencies on the first buffer in A, at least some of which occur at or after frame B end time. (Those dependent frames would no longer be preceded in playback by A, rather B would precede, and decoders would likely err/glitch.)
w.r.t.: > In math notation: Remove (prev_timestamp, next_timestamp] (the ] is a bug as > we've discussed) Please file & reference a crbug.
w.r.t.: CL description: "(TODO: Same for audio and disable splicing)." and > *Success for audio (later cl)* Is the plan to make this current CL video-specific? Or, by "later cl", do you mean "later patch set to this CL"? Note: As discussed, I'll be syncing w/you and acolwell@ later today to see what might best be proposed as track "buffered" for the non-estimated case (and if it makes sense to do the same for estimated). My CR of your PS2 will follow (probably Monday, at this point.)
On 2015/03/20 18:56:03, wolenetz wrote: > w.r.t: > > One interesting thing to remember is that if at any point in FP algorithm you > > remove a frame, you must remove all frames AFTER that frame which depend on > the > > removed frame. This doesn't affect our overlap case (as our original > overlapped > > frame is always BEFORE), but still, this is good to keep in mind. This is > spec'd > > and implemented SBS behavior. > > IIUC, it *should* affect our overlap case, if the "first append buffer A" were > changed to "first append GOP A containing multiple dependencies on the first > buffer in A, at least some of which occur at or after frame B end time. (Those > dependent frames would no longer be preceded in playback by A, rather B would > precede, and decoders would likely err/glitch.) I agree, it seems like those dependents of A should be removed in this case. Going off my recent readings of spec and our implementation, I don't think this removal of *all* dependent frames after B is in either spec or impl (though we would remove anything with a PTS that fell within B's [pts, endTs). When you say this *should* affect our overlap, I don't see that it affects the overlap / estimation approach. IIUC the GOP problem exists regardless of estimation and estimation makes it no worse/better/less-likely/more-likely. In the proposed patch, if A were found to be estimated, its duration would be trimmed back, which seems like a reasonable thing to do here just like in cases where we're just continuing an existing media segment. We would should still consider removing the dependent frames after B either way, but not as part of this change. Do you want me to file a spec bug for this?
On 2015/03/20 21:27:25, wolenetz wrote: > w.r.t.: > CL description: "(TODO: Same for audio and disable splicing)." and > > *Success for audio (later cl)* > > Is the plan to make this current CL video-specific? Or, by "later cl", do you > mean "later patch set to this CL"? > > Note: As discussed, I'll be syncing w/you and acolwell@ later today to see what > might best be proposed as track "buffered" for the non-estimated case (and if it > makes sense to do the same for estimated). My CR of your PS2 will follow > (probably Monday, at this point.) I'm planning to restrict the max-estimation to just video in this CL. Will do a followup change after I commit to handle Audio. It will need a separate test to ensure that splicing isn't improperly triggered and its a nice way to break it up.
On 2015/03/20 18:57:00, wolenetz wrote: > w.r.t.: > > In math notation: Remove (prev_timestamp, next_timestamp] (the ] is a bug as > > we've discussed) > > Please file & reference a crbug. https://code.google.com/p/chromium/issues/detail?id=469325
On 2015/03/20 21:53:35, chcunningham wrote: > On 2015/03/20 18:56:03, wolenetz wrote: > > w.r.t: > > > One interesting thing to remember is that if at any point in FP algorithm > you > > > remove a frame, you must remove all frames AFTER that frame which depend on > > the > > > removed frame. This doesn't affect our overlap case (as our original > > overlapped > > > frame is always BEFORE), but still, this is good to keep in mind. This is > > spec'd > > > and implemented SBS behavior. > > > > IIUC, it *should* affect our overlap case, if the "first append buffer A" were > > changed to "first append GOP A containing multiple dependencies on the first > > buffer in A, at least some of which occur at or after frame B end time. (Those > > dependent frames would no longer be preceded in playback by A, rather B would > > precede, and decoders would likely err/glitch.) > > I agree, it seems like those dependents of A should be removed in this case. > Going off my recent readings of spec and our implementation, I don't think this > removal of *all* dependent frames after B is in either spec or impl (though we > would remove anything with a PTS that fell within B's [pts, endTs). > > When you say this *should* affect our overlap, I don't see that it affects the > overlap / estimation approach. IIUC the GOP problem exists regardless of > estimation and estimation makes it no worse/better/less-likely/more-likely. In > the proposed patch, if A were found to be estimated, its duration would be > trimmed back, which seems like a reasonable thing to do here just like in cases > where we're just continuing an existing media segment. We would should still > consider removing the dependent frames after B either way, but not as part of > this change. Do you want me to file a spec bug for this? I added detail to an existing spec bug that I think covers these cases: https://www.w3.org/Bugs/Public/show_bug.cgi?id=27242#c3 In particular, I think we would/should have problem if we didn't remove dependencies during duration truncation: Append: [A...............)[a........] Append: [B..........) I don't think [a........] should remain buffered. If it did, decoder error/glitch on playback would occur on decode of A+B+a sequence, I think. What does current code do for this scenario? Neither do I think "A" should remain estimated, in this case, since it was immediately followed by a decode dependency "a". I know of no way for "a" to be in a buffered range without being appended continuously after "A" (either within same media segment as "A", or in next immediately appended media segment after "A" under most recent spec (which no longer requires media segments to begin with keyframe))
On 2015/03/20 21:56:25, chcunningham wrote:
> On 2015/03/20 21:27:25, wolenetz wrote:
> > w.r.t.:
> > CL description: "(TODO: Same for audio and disable splicing)." and
> > > *Success for audio (later cl)*
> >
> > Is the plan to make this current CL video-specific? Or, by "later cl", do
you
> > mean "later patch set to this CL"?
> >
> > Note: As discussed, I'll be syncing w/you and acolwell@ later today to see
> what
> > might best be proposed as track "buffered" for the non-estimated case (and
if
> it
> > makes sense to do the same for estimated). My CR of your PS2 will follow
> > (probably Monday, at this point.)
>
> I'm planning to restrict the max-estimation to just video in this CL. Will do
a
> followup change after I commit to handle Audio. It will need a separate test
to
> ensure that splicing isn't improperly triggered and its a nice way to break it
> up.
sgtm. Sync with acolwell@ last Friday didn't cover enough to decide what to do
for both non-estimated and estimated [A...] with dependent buffer [a...] as part
of appending B (see below example). This issue is orthogonal to this CL, sorry
for it slowing this CL down. Next sync w/acolwell@ is Monday.
[A............)[a........)
[B.......)
Looking pretty good. Beyond inline comments: 1. Do the layout tests still pass with this CL landed, or do they need to be temporarily disabled and fixed while this is landed? 2. Please change CL description to make explicit that this is only for improving WebM *video* frame duration estimation, and remove the "(TODO:..." part of the CL description. 3. Please add SourceBufferStreamTest(s) covering the "scaling back max estimate" portion of this CL. Adjustments to existing, or adding new test helpers may be necessary to validate the buffers' adjusted durations. https://codereview.chromium.org/1018373003/diff/40001/media/base/decoder_buff... File media/base/decoder_buffer.h (right): https://codereview.chromium.org/1018373003/diff/40001/media/base/decoder_buff... media/base/decoder_buffer.h:89: bool is_duration_estimated() { return is_duration_estimated_; } nit: const method https://codereview.chromium.org/1018373003/diff/40001/media/base/decoder_buff... media/base/decoder_buffer.h:91: void set_is_duration_estimated(bool is_estimated) { Must these be in DecoderBuffer? Or is StreamParserBuffer a better place for these for now? (Does anything downstream of SourceBufferStream care whether or not a stream parser at some point estimated this buffer's duration, even if it's been subsequently adjusted?) https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:69: void SourceBufferRange::AdjustEstimatedDurationForNewAppend( Does adjustment to duration correctly include when SBS::track_buffer_ ends with the last appended buffer and needs to have its estimated duration adjusted based on |new_buffers|? https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:71: if (buffers_.empty() || new_buffers.empty()) { nit: Change call to only occur when buffers_ is not empty. Then DCHECK both !buffers_.empty() and !new_buffers.empty() here. Note that AppendBuffersToEnd already protects against new_buffers_ being empty when buffers_ is not empty (see CanAppendBuffersToEnd, which should fail hard if the new buffers are empty). https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:75: // Only adjust the last of the previously appended buffers. The last buffer nit: s/buffers./buffers if its duration was previously estimated. https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:77: // the implicit PTS-delta duration because the do not know the PTS of the nit: s/because the/because they/.. Actually, is this too much info? Maybe reduce some of this to "If estimated, refine the estimation as necessary using the PTS of the first new buffer being appended that has been deemed to be next in this range." https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:80: scoped_refptr<StreamParserBuffer> last_appended_buffer = buffers_.back(); nit: const& the smart pointer to reduce unnecessary refcount churn locally here. https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:87: << ") from last append with derived duration (" nit: may not have been from "last" append. https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:789: if (splice_frames_enabled_) { nit: why { } here and not elsewhere like below, and why in this CL? https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:30: // durations have been estimated. nit: regrammarize "a buffer durations have" ;) https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:694: << "last (Simple)Block in the Cluster. Use BlockDuration at the end of " nit: "in the Cluster for this Track". And "Instead of SimpleBlocks, use BlockGroups with BlockDurations at the end of each Track in a Cluster to avoid estimation." https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:767: // so maximum is used and overlap is is simply resolved by showing the nit: is is https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:789: << " at dts: " nit: webm doesn't differentiate dts/pts. s/dts/timestamp/ aside: it's weird that even inside webm parser we sometimes use buffer->timestamp() and other times use buffer->GetDecodeTimestamp(). Seems ripe for some low-pri cleanup. https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.h (right): https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.h:34: kDefaultVideoBufferDurationInMs = 63 // Low 16fps to reduce stalls nit: comment here (and elaborate in CL description) that, in combination with compliant coded frame processing discontinuity detection logic, this mitigates videos with framerates as low as 8fps (max 0.125ms timestamp delta) causing gaps when this estimate is used (though not in all cases, like varying frame-rate streams, since we might have already determined a lesser estimate from previous frames).
Thanks Matt. See inline: 1. Do the layout tests still pass with this CL landed, or do they need to be temporarily disabled and fixed while this is landed? They pass, but I had to restrict the estimation adjustments to only occur for video buffers (which was my intention all along in this CL). Since SourceBufferRange has no knowledge of audio/video, I did this by only setting the is_duration_estaimted flag for video buffers. See the slight tweak and comments in WebmClusterParser. 2. Please change CL description to make explicit that this is only for improving WebM *video* frame duration estimation, and remove the "(TODO:..." part of the CL description. Done. 3. Please add SourceBufferStreamTest(s) covering the "scaling back max estimate" portion of this CL. Adjustments to existing, or adding new test helpers may be necessary to validate the buffers' adjusted durations. In progress. Will have this soon. https://codereview.chromium.org/1018373003/diff/40001/media/base/decoder_buff... File media/base/decoder_buffer.h (right): https://codereview.chromium.org/1018373003/diff/40001/media/base/decoder_buff... media/base/decoder_buffer.h:89: bool is_duration_estimated() { return is_duration_estimated_; } On 2015/03/28 00:26:05, wolenetz wrote: > nit: const method Done. https://codereview.chromium.org/1018373003/diff/40001/media/base/decoder_buff... media/base/decoder_buffer.h:91: void set_is_duration_estimated(bool is_estimated) { On 2015/03/28 00:26:05, wolenetz wrote: > Must these be in DecoderBuffer? Or is StreamParserBuffer a better place for > these for now? (Does anything downstream of SourceBufferStream care whether or > not a stream parser at some point estimated this buffer's duration, even if it's > been subsequently adjusted?) Moved to StreamParserBuffer. SBS is currently the only consumer. https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:69: void SourceBufferRange::AdjustEstimatedDurationForNewAppend( On 2015/03/28 00:26:06, wolenetz wrote: > Does adjustment to duration correctly include when SBS::track_buffer_ ends with > the last appended buffer and needs to have its estimated duration adjusted based > on |new_buffers|? This is a great question and I've given it a lot of thought. At present I am *not* adjusting the duration for the buffer at the tail of the SBS::track_buffer_. I'm thinking of leaving it this way... There are a few ways I've pondered this: 1. What is the likelihood that adjusting the track buffer like we adjust in SourceBufferedRange will correct a bad estimate (which is the whole point of the adjustment)? I would say the likelihood is zero. The buffers put into the track buffer are not just a little overlapped (due to an off estimate). They are entirely overlapped (at least their start times are) by buffers that are now sitting in our tracked ranges. 2. But maybe its worth adjusting the duration anyway? My opinion at this point is it doesn't make sense to adjust (regardless of whether the tail is estimated). I came to this conclusion after considering how we might do the adjusting. I considered searching the ranges for a buffer (lets call it NEXT_IN_TIME) who's start time most closely follows the tail of the track_buffer_. We could then just take the delta's in these start times and assign the track_buffer_'s tail this delta as its duration. The thing is, not only is that probably not the encoded duration for the buffer, but also, its not even consistent with how the timeline behaves after we've exhausted the track buffer. As you know, once the track buffer is exhausted the next frame we play is the next KEY frame in our ranges who's DTS follows the track buffer's last DTS. This key frame may be any number of frames apart from that NEXT_IN_TIME buffer we used to compute the duration. little diagram: https://drive.google.com/file/d/0BwlI4ijQnVJRa3l5VUZ4Y2xONFk/view?usp=sharing https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:71: if (buffers_.empty() || new_buffers.empty()) { On 2015/03/28 00:26:06, wolenetz wrote: > nit: Change call to only occur when buffers_ is not empty. Then DCHECK both > !buffers_.empty() and !new_buffers.empty() here. Note that AppendBuffersToEnd > already protects against new_buffers_ being empty when buffers_ is not empty > (see CanAppendBuffersToEnd, which should fail hard if the new buffers are > empty). Let me push back a little here (or at least argue the other side). I think the way I have it is a simpler contract. Its easy to reason about, callers have less overhead this way, and its overall less lines of code. If you see major upside in the other approach LMK. In classes that are this complex I'm sensitive to having contracts like "dont call if empty" which are subject subtly breaking in some later refactoring https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:75: // Only adjust the last of the previously appended buffers. The last buffer On 2015/03/28 00:26:05, wolenetz wrote: > nit: s/buffers./buffers if its duration was previously estimated. Done. https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:77: // the implicit PTS-delta duration because the do not know the PTS of the On 2015/03/28 00:26:05, wolenetz wrote: > nit: s/because the/because they/.. Actually, is this too much info? Maybe reduce > some of this to "If estimated, refine the estimation as necessary using the PTS > of the first new buffer being appended that has been deemed to be next in this > range." Done. Changed a little from what you wrote, but did make it much shorter. I'm hoping that the reason for focusing on the last of the estimated buffers is well documented. https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:80: scoped_refptr<StreamParserBuffer> last_appended_buffer = buffers_.back(); On 2015/03/28 00:26:05, wolenetz wrote: > nit: const& the smart pointer to reduce unnecessary refcount churn locally here. Done. Made auto too. https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:87: << ") from last append with derived duration (" On 2015/03/28 00:26:06, wolenetz wrote: > nit: may not have been from "last" append. Done. Now reads "Replacing estimated duration (X) from previous range-end with derived duration (Y)." https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:789: if (splice_frames_enabled_) { On 2015/03/28 00:26:06, wolenetz wrote: > nit: why { } here and not elsewhere like below, and why in this CL? Reverted. I see this file has plenty of single liners, so its good to be consistent. I hate them, but I appear to be in the minority. https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:30: // durations have been estimated. On 2015/03/28 00:26:06, wolenetz wrote: > nit: regrammarize "a buffer durations have" ;) Done. https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:694: << "last (Simple)Block in the Cluster. Use BlockDuration at the end of " On 2015/03/28 00:26:06, wolenetz wrote: > nit: "in the Cluster for this Track". And "Instead of SimpleBlocks, use > BlockGroups with BlockDurations at the end of each Track in a Cluster to avoid > estimation." Done. https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:767: // so maximum is used and overlap is is simply resolved by showing the On 2015/03/28 00:26:06, wolenetz wrote: > nit: is is Done. https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:789: << " at dts: " On 2015/03/28 00:26:06, wolenetz wrote: > nit: webm doesn't differentiate dts/pts. s/dts/timestamp/ > aside: it's weird that even inside webm parser we sometimes use > buffer->timestamp() and other times use buffer->GetDecodeTimestamp(). Seems ripe > for some low-pri cleanup. Done. Re-cleanup: so you'd like a bug to change all DecodeTimestamp -> Timestamp ? I don't mind the using DecodeTimestamp, since thats intuitively whats most important to sequencing buffers in a parser and the fact that WebM doesn't differentiate is not obvious. If it were me, I'd always use DecodeTimestmp https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.h (right): https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.h:34: kDefaultVideoBufferDurationInMs = 63 // Low 16fps to reduce stalls On 2015/03/28 00:26:06, wolenetz wrote: > nit: comment here (and elaborate in CL description) that, in combination with > compliant coded frame processing discontinuity detection logic, this mitigates > videos with framerates as low as 8fps (max 0.125ms timestamp delta) causing gaps > when this estimate is used (though not in all cases, like varying frame-rate > streams, since we might have already determined a lesser estimate from previous > frames). Done.
Nice work! lgtm % removing the defunct DecoderBuffer |is_duration_estimated_| member and fixing a few new nits. Note: An edge case regarding layout tests is that we need to retain both WebM and MP4 functionality if we are trying to claim interop on MSE API (which we are working towards). WebM is tested by default in Chrom* layout tests. It's possible our MP4 test compliance is rotting over time. This is somewhat orthogonal to this CL, but worth considering as part of a layout-test-impacting CL workflow might be to disable webm in stream_parser_factory locally when seeing if the MP4 MSE implementation is still functioning correctly (and you'll need to include proprietary codecs in your local build, too). I'm thinking this needs some layout test infra/util improvement to automate, but for now, it's been manual. I've filed crbug 477175 to track this long-known issue. https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:69: void SourceBufferRange::AdjustEstimatedDurationForNewAppend( > 1. What is the likelihood that adjusting the track buffer like we adjust in > SourceBufferedRange will correct a bad estimate (which is the whole point of the > adjustment)? I would say the likelihood is zero. I agree. > 2. ... I agree. Thanks for looking into this track_buffer_ case. https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_range.cc:71: if (buffers_.empty() || new_buffers.empty()) { On 2015/04/13 23:25:18, chcunningham wrote: > On 2015/03/28 00:26:06, wolenetz wrote: > > nit: Change call to only occur when buffers_ is not empty. Then DCHECK both > > !buffers_.empty() and !new_buffers.empty() here. Note that AppendBuffersToEnd > > already protects against new_buffers_ being empty when buffers_ is not empty > > (see CanAppendBuffersToEnd, which should fail hard if the new buffers are > > empty). > > Let me push back a little here (or at least argue the other side). I think the > way I have it is a simpler contract. Its easy to reason about, callers have less > overhead this way, and its overall less lines of code. If you see major upside > in the other approach LMK. > > In classes that are this complex I'm sensitive to having contracts like "dont > call if empty" which are subject subtly breaking in some later refactoring That's fine. If this method really needed to require non-empty buffers, then DCHECK makes sense. Otherwise, it was only a nit :) https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1018373003/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:789: if (splice_frames_enabled_) { On 2015/04/13 23:25:18, chcunningham wrote: > On 2015/03/28 00:26:06, wolenetz wrote: > > nit: why { } here and not elsewhere like below, and why in this CL? > > Reverted. I see this file has plenty of single liners, so its good to be > consistent. I hate them, but I appear to be in the minority. No, I personally hope you're not in the minority on this. https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/1018373003/diff/40001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:789: << " at dts: " On 2015/04/13 23:25:18, chcunningham wrote: > On 2015/03/28 00:26:06, wolenetz wrote: > > nit: webm doesn't differentiate dts/pts. s/dts/timestamp/ > > aside: it's weird that even inside webm parser we sometimes use > > buffer->timestamp() and other times use buffer->GetDecodeTimestamp(). Seems > ripe > > for some low-pri cleanup. > > Done. Re-cleanup: so you'd like a bug to change all DecodeTimestamp -> Timestamp > ? I don't mind the using DecodeTimestamp, since thats intuitively whats most > important to sequencing buffers in a parser and the fact that WebM doesn't > differentiate is not obvious. If it were me, I'd always use DecodeTimestmp Thanks. IIRC, WebM container doesn't mention PTS or DTS, just timestamp. Internally here, it would be better to be consistent in usage of DTS or PTS on the |buffer| accessor, but that's for a really low pri cleanup :) https://codereview.chromium.org/1018373003/diff/80001/media/base/decoder_buff... File media/base/decoder_buffer.h (right): https://codereview.chromium.org/1018373003/diff/80001/media/base/decoder_buff... media/base/decoder_buffer.h:189: bool is_duration_estimated_; Please remove this (the member is now in StreamParserBuffer). https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:469: // Indicates the buffer with timestamp ## reflects a keyframe. ## nit: drop the last ## on this line and the remainder of this comment. Maybe rework this whole comment slightly to use an overall syntax descriptor that's less prone to rotting like this. Maybe like: pp[|dd][Dzz][E][P][K] much like a *nix man page :) I'm not sure similarly reworking the S(...) portion of the comment though is worth it. https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4012: CheckExpectedBuffers("0K 5 10 20D5E"); nit: and CheckNoNextBuffer(); https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4023: // Append a new buffers, where the last has estimated duration that overlaps nit: "a new buffers" ? https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4029: CheckExpectedBuffers("0K 5D5E 10K 15 20"); nit: and CheckNoNextBuffer(); https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4036: CheckExpectedBuffers("0K 5D5E"); nit ditto https://codereview.chromium.org/1018373003/diff/80001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/1018373003/diff/80001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:705: << "Estimating WebM duration to be " << estimated_duration << " for the " nit: WebM block ? https://codereview.chromium.org/1018373003/diff/80001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.h (right): https://codereview.chromium.org/1018373003/diff/80001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.h:35: // Chosen to represent 16fps duration, which will prevent MSE stalls in nit: insert blank line to break the two comment+item entries apart visually?
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Couple Qs, but otherwise lgtm https://codereview.chromium.org/1018373003/diff/80001/media/base/stream_parse... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/1018373003/diff/80001/media/base/stream_parse... media/base/stream_parser_buffer.cc:157: Forward this new flag? Or DCHECK it's not present? https://codereview.chromium.org/1018373003/diff/80001/media/base/stream_parse... media/base/stream_parser_buffer.cc:195: Any DCHECKS() you want to add here? https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_range.cc:71: if (buffers_.empty() || new_buffers.empty()) { Typically no {} for one line if statements, but up to you. https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_range.cc:81: new_buffers.front()->timestamp() - last_appended_buffer->timestamp(); Always positive? What happens in an overlap case?
Thanks for the review Dale/Matt! Matt, re mp4 tests: Will definitely keep mp4 in mind going forward. I'm a little confused why we would bother to disable webm in that workflow? How does webm support affect the mp4 tests? https://codereview.chromium.org/1018373003/diff/80001/media/base/decoder_buff... File media/base/decoder_buffer.h (right): https://codereview.chromium.org/1018373003/diff/80001/media/base/decoder_buff... media/base/decoder_buffer.h:189: bool is_duration_estimated_; On 2015/04/15 02:55:23, wolenetz wrote: > Please remove this (the member is now in StreamParserBuffer). Done. https://codereview.chromium.org/1018373003/diff/80001/media/base/stream_parse... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/1018373003/diff/80001/media/base/stream_parse... media/base/stream_parser_buffer.cc:157: Done, added DCHECKs https://codereview.chromium.org/1018373003/diff/80001/media/base/stream_parse... media/base/stream_parser_buffer.cc:195: On 2015/04/15 22:51:46, DaleCurtis wrote: > Any DCHECKS() you want to add here? I think estimation should be ok for pre-roll. Unlike splicing, pre-roll is not expected to be overlapped. Also, I can imagine a webm stream that contains estimated pre-roll, for which the estimation (and refinement in SourceBufferStream) should work well just as it does in the non-preroll case. https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_range.cc:71: if (buffers_.empty() || new_buffers.empty()) { On 2015/04/15 22:51:46, DaleCurtis wrote: > Typically no {} for one line if statements, but up to you. I'm a believer! I think no brackets is asking for trouble (and its bitten me before). https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_range.cc:81: new_buffers.front()->timestamp() - last_appended_buffer->timestamp(); On 2015/04/15 22:51:46, DaleCurtis wrote: > Always positive? What happens in an overlap case? This should be always positive, but I think this is a great place for a DCHECK (just added). For this to be negative, the buffers we are now appending would have to have a front PTS that is lower than our current back buffer PTS. AppendBuffersToEnd DCHECKS this when it calls CanAppendToEnd, and SourceBufferStream also checks using that same CanAppendToEnd before doing the append. In cases where the PTS of new buffers would overlap the PTS of existing buffers, this is handled in SourceBufferStream in the ::PrepareRangesForNextAppend, where the existing buffers which are about to be overlapped are entirely removed. https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:469: // Indicates the buffer with timestamp ## reflects a keyframe. ## On 2015/04/15 02:55:23, wolenetz wrote: > nit: drop the last ## on this line and the remainder of this comment. Maybe > rework this whole comment slightly to use an overall syntax descriptor that's > less prone to rotting like this. Maybe like: > pp[|dd][Dzz][E][P][K] > much like a *nix man page :) > > I'm not sure similarly reworking the S(...) portion of the comment though is > worth it. Done. https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4012: CheckExpectedBuffers("0K 5 10 20D5E"); On 2015/04/15 02:55:23, wolenetz wrote: > nit: and CheckNoNextBuffer(); Done. https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4023: // Append a new buffers, where the last has estimated duration that overlaps On 2015/04/15 02:55:23, wolenetz wrote: > nit: "a new buffers" ? Done. https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4029: CheckExpectedBuffers("0K 5D5E 10K 15 20"); On 2015/04/15 02:55:23, wolenetz wrote: > nit: and CheckNoNextBuffer(); Done. https://codereview.chromium.org/1018373003/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4036: CheckExpectedBuffers("0K 5D5E"); On 2015/04/15 02:55:23, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1018373003/diff/80001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/1018373003/diff/80001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:705: << "Estimating WebM duration to be " << estimated_duration << " for the " On 2015/04/15 02:55:23, wolenetz wrote: > nit: WebM block ? Done. https://codereview.chromium.org/1018373003/diff/80001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.h (right): https://codereview.chromium.org/1018373003/diff/80001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.h:35: // Chosen to represent 16fps duration, which will prevent MSE stalls in On 2015/04/15 02:55:23, wolenetz wrote: > nit: insert blank line to break the two comment+item entries apart visually? Done.
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/1018373003/#ps120001 (title: "Feedback, round 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1018373003/120001
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/1018373003/#ps140001 (title: "Fixing try failure, remove unused variable for some builds.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1018373003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/985c1f983bf4092dc5504a3c363b3fe2a535c03a Cr-Commit-Position: refs/heads/master@{#325507}
Message was sent while issue was closed.
On 2015/04/16 18:04:16, chcunningham wrote:
> Matt, re mp4 tests: Will definitely keep mp4 in mind going forward. I'm a
little
> confused why we would bother to disable webm in that workflow? How does webm
> support affect the mp4 tests?
Though I'm considering reworking the test structure a bit to test all of {webm,
mp4, mp2ts, etc}, if available in the build, the current structure of the
mediasource_utils.js affords only one of {webm, mp4} byte streams in the test
(and it picks the first that is supported). Hence, to test mp4 in Chrome using
these layout tests as currently constructed, you need to disable support for
webm in the build. This is most easily hacked locally by removing webm stream
parser support in stream_parser_factory.cc.
My rework is motivated and likely due to upcoming push to get MSE tested for
interop across various browsers (so the spec can move out of the CR w3c phase)
and the likelihood that the interop tests will continue to be based on the
structure of our existing mediasource layout tests.
So, for short term, to test mp4 in MSE in Chrome, must locally disable webm
support in Chrome and build Chrome with mp4 locally.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
