Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1347483003:
Fix seeking back in the new MSE GC algorithm (Closed)
DescriptionFix seeking back in the new MSE GC algorithm
BUG=532136
Committed: https://crrev.com/e41e206081c2996cce1385efef219fa3fc0c2581
Cr-Commit-Position: refs/heads/master@{#350104}
Patch Set 1 #Patch Set 2 : better fix #
Total comments: 11
Patch Set 3 : Allow GC to delete as much data from the front as necessary #Patch Set 4 : Reworked to accomodate CR feedback #Patch Set 5 : upd #
Total comments: 2
Patch Set 6 : Latest patchset from 1349973002 which addressed feedback comments made there #
Total comments: 11
Patch Set 7 : CR feedback #
Total comments: 4
Patch Set 8 : Fixed typo #Patch Set 9 : Nit: removed incorrect comment #Patch Set 10 : Rebase to ToT #
Messages
Total messages: 38 (12 generated)
servolk@chromium.org changed reviewers: + wolenetz@chromium.org
Looking good. Please add a little detail to the commit message like: Allows more aggressive garbage collection from the front of the first buffered range if there is a pending seek to a time prior to buffered media. https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:762: // If media_time is before the first buffered range, that means we are in the nit:s/media_time/|media_time|/ https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:763: // process of seeking back in the stream. HTMLMediaElement adjusts media_time nit: s/HTMLMediaElement/caller/ nit: s/media_time/|media_time|/ https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:774: media_time = ranges_.front()->GetEndTimestamp(); nit: While this looks like it would work, could the change be more focused on a different condition in l.795 to help maintainability? I tried briefly to contrive such a condition, but it ended up being more complex. WDYT? https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:776: Please add a unit test that verifies this works as intended. Especially, if modifying l.795, make sure that the invariant that media_time isn't changed after l.775 is held true. (Keeping to that invariant was what complicated my attempt to construct a condition localized in just l.795).
https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:775: } As discussed further in chat, it seems we should allow collection (from front) of as much as is needed, and not stop at the end of the first buffered range (there could be many). Also, updating media_time here smells funny (I think to both of us). Please change this logic to instead set some boolean like "greedy_collect_from_front" true and use that to prevent stopping the collection in l.795. Yes, we could collect from the most recently appended range, including the most recent append. Given that there might be a distinct collectable range after the most recently appended-to range, in this case, we should collect from back first (until last appended buffer), and then greedily collect from front? (I know the caller determines the sequence of collect from front/back/etc, so the caller might need some similar conditioning.)
https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:661: w.r.t. my comment on l.775: maybe do something like: if (unsatisfied seek is pending prior to the last appended buffer/range), then first collect from back (until reaching the GOP that was last appended to) and then greedy collect from front; otherwise do the current code sequence (non-greedy from front, normal from back).
https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:762: // If media_time is before the first buffered range, that means we are in the On 2015/09/15 22:36:13, wolenetz wrote: > nit:s/media_time/|media_time|/ Done. https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:763: // process of seeking back in the stream. HTMLMediaElement adjusts media_time On 2015/09/15 22:36:13, wolenetz wrote: > nit: s/HTMLMediaElement/caller/ > nit: s/media_time/|media_time|/ Done. https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:774: media_time = ranges_.front()->GetEndTimestamp(); On 2015/09/15 22:36:12, wolenetz wrote: > nit: While this looks like it would work, could the change be more focused on a > different condition in l.795 to help maintainability? I tried briefly to > contrive such a condition, but it ended up being more complex. WDYT? Acknowledged. I've reworked this as we discussed over hangouts. https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:775: } On 2015/09/15 23:04:51, wolenetz wrote: > As discussed further in chat, it seems we should allow collection (from front) > of as much as is needed, and not stop at the end of the first buffered range > (there could be many). Also, updating media_time here smells funny (I think to > both of us). Please change this logic to instead set some boolean like > "greedy_collect_from_front" true and use that to prevent stopping the collection > in l.795. > > Yes, we could collect from the most recently appended range, including the most > recent append. Given that there might be a distinct collectable range after the > most recently appended-to range, in this case, we should collect from back first > (until last appended buffer), and then greedily collect from front? (I know the > caller determines the sequence of collect from front/back/etc, so the caller > might need some similar conditioning.) Done in the lastest patchset. https://codereview.chromium.org/1347483003/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:776: On 2015/09/15 22:36:12, wolenetz wrote: > Please add a unit test that verifies this works as intended. > Especially, if modifying l.795, make sure that the invariant that media_time > isn't changed after l.775 is held true. (Keeping to that invariant was what > complicated my attempt to construct a condition localized in just l.795). I've added a ChunkDemuxer unit tests that verifies that GC succeeds when seeking to a position that is earlier than any buffered ranges.
I would like expanding the greediness a bit to any time the seek time is unsatisfied (not just if before *any* buffered range, but also be greedy if seeking to a time between buffered ranges), and adding a bit more unit test coverage: https://codereview.chromium.org/1347483003/diff/80001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1347483003/diff/80001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3425: EXPECT_TRUE(demuxer_->EvictCodedFrames(kSourceId, seek_time, 5 * kBlockSize)); nit: We could do a further check of expectation here that the resulting buffered ranges after GC of 5 blocks is now { [ 1115,1230) } IIUC. Also, it would be good to have a variety of related GC-while-unsatisfied-seek tests: 1) this one 2) three buffered ranges, seek time is between first and second, last append is at end of third range; ensure that GC can collect from front into the middle of the third range (or maybe a 2b. case that also tests that *all* could be collected). 3) a couple buffered ranges with seek time between them and last appended buffer was at end of the *first* range. I'm probably missing one or two major test details here, but you get the point: I don't think we previously had nearly enough unsatisfied-seek-and-GC test coverage. This one test is great; I would like more. https://codereview.chromium.org/1347483003/diff/80001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1347483003/diff/80001/media/filters/source_bu... media/filters/source_buffer_stream.cc:662: // If |media_time| is earlier than any of the buffered data, that means we are I wonder if we should make this even more aggressive: Suppose buffered: [0,10] [20,30] [40,50] Last append: 50 Seek==media time: 15 GC requested such that bytes to free implies maybe just 5 seconds buffered time could remain after successful eviction (for simplicity; I know we talk bytes, but in this example I'm talking seconds). In this PS, [20,30] and [40,50] would survive because the 'else' clause won't collect between media_time and last_append (where media_time < last_append), and GC would fail. Yet I think we could be greedy here instead. WDYT? Proposed change to l.669 in this PS would be something like if (!ranges_.empty() && (media time is not in any buffered range)) { be greedy }else{ be conservative }
On 2015/09/16 19:25:27, wolenetz wrote: > I would like expanding the greediness a bit to any time the seek time is > unsatisfied (not just if before *any* buffered range, but also be greedy if > seeking to a time between buffered ranges), and adding a bit more unit test > coverage: > > https://codereview.chromium.org/1347483003/diff/80001/media/filters/chunk_dem... > File media/filters/chunk_demuxer_unittest.cc (right): > > https://codereview.chromium.org/1347483003/diff/80001/media/filters/chunk_dem... > media/filters/chunk_demuxer_unittest.cc:3425: > EXPECT_TRUE(demuxer_->EvictCodedFrames(kSourceId, seek_time, 5 * kBlockSize)); > nit: We could do a further check of expectation here that the resulting buffered > ranges after GC of 5 blocks is now { [ 1115,1230) } IIUC. > > Also, it would be good to have a variety of related GC-while-unsatisfied-seek > tests: > 1) this one > 2) three buffered ranges, seek time is between first and second, last append is > at end of third range; ensure that GC can collect from front into the middle of > the third range (or maybe a 2b. case that also tests that *all* could be > collected). > 3) a couple buffered ranges with seek time between them and last appended buffer > was at end of the *first* range. > > I'm probably missing one or two major test details here, but you get the point: > I don't think we previously had nearly enough unsatisfied-seek-and-GC test > coverage. This one test is great; I would like more. > > https://codereview.chromium.org/1347483003/diff/80001/media/filters/source_bu... > File media/filters/source_buffer_stream.cc (right): > > https://codereview.chromium.org/1347483003/diff/80001/media/filters/source_bu... > media/filters/source_buffer_stream.cc:662: // If |media_time| is earlier than > any of the buffered data, that means we are > I wonder if we should make this even more aggressive: > > Suppose buffered: > [0,10] [20,30] [40,50] > > Last append: 50 > Seek==media time: 15 > GC requested such that bytes to free implies maybe just 5 seconds buffered time > could remain after successful eviction (for simplicity; I know we talk bytes, > but in this example I'm talking seconds). > > In this PS, [20,30] and [40,50] would survive because the 'else' clause won't > collect between media_time and last_append (where media_time < last_append), and > GC would fail. Yet I think we could be greedy here instead. > > WDYT? Proposed change to l.669 in this PS would be something like > if (!ranges_.empty() && > (media time is not in any buffered range)) { be greedy }else{ be > conservative } Yes, I think we can allow the GC algorithm to be more aggressive if we detect an unsatisfied pending seek, although that turned out to be a bigger change than I expected, so I've uploaded it as a separate CL for now (with new unit tests), PTAL at https://codereview.chromium.org/1349973002 Re the tests - I'm not sure how to handle all the possible cases with unit tests, since the number of possible input states (number buffered ranges, media_time position, seek target position, last append position) is exploding, I'm not sure how many of those states we actually encounter/care about in reality).
On 2015/09/17 02:33:07, servolk wrote: > On 2015/09/16 19:25:27, wolenetz wrote: > > I would like expanding the greediness a bit to any time the seek time is > > unsatisfied (not just if before *any* buffered range, but also be greedy if > > seeking to a time between buffered ranges), and adding a bit more unit test > > coverage: > > > > > https://codereview.chromium.org/1347483003/diff/80001/media/filters/chunk_dem... > > File media/filters/chunk_demuxer_unittest.cc (right): > > > > > https://codereview.chromium.org/1347483003/diff/80001/media/filters/chunk_dem... > > media/filters/chunk_demuxer_unittest.cc:3425: > > EXPECT_TRUE(demuxer_->EvictCodedFrames(kSourceId, seek_time, 5 * kBlockSize)); > > nit: We could do a further check of expectation here that the resulting > buffered > > ranges after GC of 5 blocks is now { [ 1115,1230) } IIUC. > > > > Also, it would be good to have a variety of related GC-while-unsatisfied-seek > > tests: > > 1) this one > > 2) three buffered ranges, seek time is between first and second, last append > is > > at end of third range; ensure that GC can collect from front into the middle > of > > the third range (or maybe a 2b. case that also tests that *all* could be > > collected). > > 3) a couple buffered ranges with seek time between them and last appended > buffer > > was at end of the *first* range. > > > > I'm probably missing one or two major test details here, but you get the > point: > > I don't think we previously had nearly enough unsatisfied-seek-and-GC test > > coverage. This one test is great; I would like more. > > > > > https://codereview.chromium.org/1347483003/diff/80001/media/filters/source_bu... > > File media/filters/source_buffer_stream.cc (right): > > > > > https://codereview.chromium.org/1347483003/diff/80001/media/filters/source_bu... > > media/filters/source_buffer_stream.cc:662: // If |media_time| is earlier than > > any of the buffered data, that means we are > > I wonder if we should make this even more aggressive: > > > > Suppose buffered: > > [0,10] [20,30] [40,50] > > > > Last append: 50 > > Seek==media time: 15 > > GC requested such that bytes to free implies maybe just 5 seconds buffered > time > > could remain after successful eviction (for simplicity; I know we talk bytes, > > but in this example I'm talking seconds). > > > > In this PS, [20,30] and [40,50] would survive because the 'else' clause won't > > collect between media_time and last_append (where media_time < last_append), > and > > GC would fail. Yet I think we could be greedy here instead. > > > > WDYT? Proposed change to l.669 in this PS would be something like > > if (!ranges_.empty() && > > (media time is not in any buffered range)) { be greedy }else{ be > > conservative } > > Yes, I think we can allow the GC algorithm to be more aggressive if we detect an > unsatisfied pending seek, although that turned out to be a bigger change than I > expected, so I've uploaded it as a separate CL for now (with new unit tests), > PTAL at https://codereview.chromium.org/1349973002 > Re the tests - I'm not sure how to handle all the possible cases with unit > tests, since the number of possible input states (number buffered ranges, > media_time position, seek target position, last append position) is exploding, > I'm not sure how many of those states we actually encounter/care about in > reality). Exhaustive tests may be needed eventually if player GC problems persist beyond this CL. I like the new tests in https://codereview.chromium.org/1349973002/, and have requested just one more test there. As mentioned in my comments on https://codereview.chromium.org/1349973002/, expectation is that that CL will likely become the next patch set (6?) of this current CL.
On 2015/09/17 18:59:18, wolenetz wrote: > On 2015/09/17 02:33:07, servolk wrote: > > On 2015/09/16 19:25:27, wolenetz wrote: > > > I would like expanding the greediness a bit to any time the seek time is > > > unsatisfied (not just if before *any* buffered range, but also be greedy if > > > seeking to a time between buffered ranges), and adding a bit more unit test > > > coverage: > > > > > > > > > https://codereview.chromium.org/1347483003/diff/80001/media/filters/chunk_dem... > > > File media/filters/chunk_demuxer_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/1347483003/diff/80001/media/filters/chunk_dem... > > > media/filters/chunk_demuxer_unittest.cc:3425: > > > EXPECT_TRUE(demuxer_->EvictCodedFrames(kSourceId, seek_time, 5 * > kBlockSize)); > > > nit: We could do a further check of expectation here that the resulting > > buffered > > > ranges after GC of 5 blocks is now { [ 1115,1230) } IIUC. > > > > > > Also, it would be good to have a variety of related > GC-while-unsatisfied-seek > > > tests: > > > 1) this one > > > 2) three buffered ranges, seek time is between first and second, last append > > is > > > at end of third range; ensure that GC can collect from front into the middle > > of > > > the third range (or maybe a 2b. case that also tests that *all* could be > > > collected). > > > 3) a couple buffered ranges with seek time between them and last appended > > buffer > > > was at end of the *first* range. > > > > > > I'm probably missing one or two major test details here, but you get the > > point: > > > I don't think we previously had nearly enough unsatisfied-seek-and-GC test > > > coverage. This one test is great; I would like more. > > > > > > > > > https://codereview.chromium.org/1347483003/diff/80001/media/filters/source_bu... > > > File media/filters/source_buffer_stream.cc (right): > > > > > > > > > https://codereview.chromium.org/1347483003/diff/80001/media/filters/source_bu... > > > media/filters/source_buffer_stream.cc:662: // If |media_time| is earlier > than > > > any of the buffered data, that means we are > > > I wonder if we should make this even more aggressive: > > > > > > Suppose buffered: > > > [0,10] [20,30] [40,50] > > > > > > Last append: 50 > > > Seek==media time: 15 > > > GC requested such that bytes to free implies maybe just 5 seconds buffered > > time > > > could remain after successful eviction (for simplicity; I know we talk > bytes, > > > but in this example I'm talking seconds). > > > > > > In this PS, [20,30] and [40,50] would survive because the 'else' clause > won't > > > collect between media_time and last_append (where media_time < last_append), > > and > > > GC would fail. Yet I think we could be greedy here instead. > > > > > > WDYT? Proposed change to l.669 in this PS would be something like > > > if (!ranges_.empty() && > > > (media time is not in any buffered range)) { be greedy }else{ be > > > conservative } > > > > Yes, I think we can allow the GC algorithm to be more aggressive if we detect > an > > unsatisfied pending seek, although that turned out to be a bigger change than > I > > expected, so I've uploaded it as a separate CL for now (with new unit tests), > > PTAL at https://codereview.chromium.org/1349973002 > > Re the tests - I'm not sure how to handle all the possible cases with unit > > tests, since the number of possible input states (number buffered ranges, > > media_time position, seek target position, last append position) is exploding, > > I'm not sure how many of those states we actually encounter/care about in > > reality). > > Exhaustive tests may be needed eventually if player GC problems persist beyond > this CL. > I like the new tests in https://codereview.chromium.org/1349973002/, and have > requested just one more test there. > As mentioned in my comments on https://codereview.chromium.org/1349973002/, > expectation is that that CL will likely become the next patch set (6?) of this > current CL. Yes, as I've explained offline the goal of making that CL a separate one, instead of a new patchset in this CL was to show you what the changes would look like to see if you agree with that. Now I've addressed the comments that you made on that CL and uploaded the same git commit as the latest patchset in both CLs. I guess we can continue the review here, since this CL has more history/context.
Looking pretty good. Just a few clean-ups/clarifications left, I think: https://codereview.chromium.org/1347483003/diff/100001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1347483003/diff/100001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:3465: base::TimeDelta seek_time = base::TimeDelta::FromMilliseconds(1500); hmm. comment seems strange: how could we *not* signal a seek, yet media_time be somewhere outside a currently buffered range? What exactly are we trying to test here? https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:38: const int kMaxGarbageCollectAlgorithWarningLogs = 50; nit: 50 is probably too high. In general, GC failure should be allowed for, so we don't want to allow repeated such failures to exhaust the media internals event cache. so, s/50/20/ please. https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:628: << "GarbageCollectIfNeeded failed: memory_limit_=" << memory_limit_ nit: since this is going to chrome://media-internals, let's help developers out by letting them know at least the type of the stream (audio, video, ...) in the log message, as well as a little more plainly state the problem. Something like "New [audio,video,text] append of size [x] bytes exceeds buffering capacity of [y] bytes. Previously buffered size was [z] bytes." https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:641: bool seek_pending = IsSeekPending(); IIUC, IsSeekPending() isn't really what we want: it allows for edge cases related to EOS to filter the |seek_pending_| state. Also, "seek_pending" looks too much like "seek_pending_" but has different meaning. Finally, IIUC, Blink always unmarks EOS prior to reaching this GC code path, so |seek_pending_| should be exactly what we want to use here. So, please use |seek_pending_| instead of a local, and also add a DCHECK(!end_of_stream_); here (perhaps with a clarifying comment why...) https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:735: return bytes_freed >= bytes_to_free; nit: I recommend adding another LIMITED_MEDIA_LOG(DEBUG...) here, capped by a different counter (also with cap, maybe of just 5), for when bytes_freed < bytes_to_free.
https://codereview.chromium.org/1347483003/diff/100001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1347483003/diff/100001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:3465: base::TimeDelta seek_time = base::TimeDelta::FromMilliseconds(1500); On 2015/09/21 20:14:39, wolenetz wrote: > hmm. comment seems strange: how could we *not* signal a seek, yet media_time be > somewhere outside a currently buffered range? What exactly are we trying to test > here? Oops, copy/paste mistake, sorry. The scenario we want to check in this test case is the one where playback is happening in some currently buffered range and then the app starts appending data outside of the currently playing range (I've seen Netflix doing that) in preparation to seeking, but doesn't issue the actual seek request yet, i.e. doesn't set HTMLMediaElement.position. In those cases we want to try and preserve data in the most recently appended range, since the app is probably going to seek to that range after pre-buffering enough data. https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:38: const int kMaxGarbageCollectAlgorithWarningLogs = 50; On 2015/09/21 20:14:39, wolenetz wrote: > nit: 50 is probably too high. In general, GC failure should be allowed for, so > we don't want to allow repeated such failures to exhaust the media internals > event cache. > > so, s/50/20/ please. Done. https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:628: << "GarbageCollectIfNeeded failed: memory_limit_=" << memory_limit_ On 2015/09/21 20:14:39, wolenetz wrote: > nit: since this is going to chrome://media-internals, let's help developers out > by letting them know at least the type of the stream (audio, video, ...) in the > log message, as well as a little more plainly state the problem. Something like > > "New [audio,video,text] append of size [x] bytes exceeds buffering capacity of > [y] bytes. Previously buffered size was [z] bytes." Done. https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:641: bool seek_pending = IsSeekPending(); On 2015/09/21 20:14:39, wolenetz wrote: > IIUC, IsSeekPending() isn't really what we want: it allows for edge cases > related to EOS to filter the |seek_pending_| state. Also, "seek_pending" looks > too much like "seek_pending_" but has different meaning. > Finally, IIUC, Blink always unmarks EOS prior to reaching this GC code path, so > |seek_pending_| should be exactly what we want to use here. > So, please use |seek_pending_| instead of a local, and also add a > DCHECK(!end_of_stream_); here (perhaps with a clarifying comment why...) Done. https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:735: return bytes_freed >= bytes_to_free; On 2015/09/21 20:14:39, wolenetz wrote: > nit: I recommend adding another LIMITED_MEDIA_LOG(DEBUG...) here, capped by a > different counter (also with cap, maybe of just 5), for when bytes_freed < > bytes_to_free. Actually I think in this case we should use (D)VLOG instead of LIMITED_MEDIA_LOG. First of all this shouldn't happen very often in properly written apps and if it does happen many times (e.g. during a very long media playback) we want to see all instances where this has occured and now just the first few ones. Also just seeing this in the media log is not that useful without seeing the rest of the context (how much data was buffered before, what exactly the GC algorithm attempted to do) which is currenty logged via DVLOG. So using MEDIA_LOG here is probably not that helpful, please let me know if you disagree.
lgtm % two tiny nits Thanks for fixing this! (Also, it would be good to try to repro b/24066260 to see if the unmodified original player is fixed with this change. Results, of course, can be reported within that b/.) https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... media/filters/source_buffer_stream.cc:735: return bytes_freed >= bytes_to_free; On 2015/09/21 21:09:57, servolk wrote: > On 2015/09/21 20:14:39, wolenetz wrote: > > nit: I recommend adding another LIMITED_MEDIA_LOG(DEBUG...) here, capped by a > > different counter (also with cap, maybe of just 5), for when bytes_freed < > > bytes_to_free. > > Actually I think in this case we should use (D)VLOG instead of > LIMITED_MEDIA_LOG. First of all this shouldn't happen very often in properly > written apps and if it does happen many times (e.g. during a very long media > playback) we want to see all instances where this has occured and now just the > first few ones. Also just seeing this in the media log is not that useful > without seeing the rest of the context (how much data was buffered before, what > exactly the GC algorithm attempted to do) which is currenty logged via DVLOG. So > using MEDIA_LOG here is probably not that helpful, please let me know if you > disagree. Acknowledged. Except in the iteration of the stream append loop, quota exceeded exception is also given to the app synchronously on an append's failure to evict enough to stay under the limit, so we have quite a bit of state already available to the developer. So existing DVLOG is ok for now. I'm strongly considering expanding upon the decode error enumeration in the HTMLMediaElement API so that more granular, *and*, standardized error codes for things like this can be exposed. Of course, that's outside the scope of this CL :) https://codereview.chromium.org/1347483003/diff/120001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1347483003/diff/120001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:3468: // Append data to complete seek operation nit: nix this comment line since we're not seeking? https://codereview.chromium.org/1347483003/diff/120001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1347483003/diff/120001/media/filters/source_b... media/filters/source_buffer_stream.cc:38: const int kMaxGarbageCollectAlgorithWarningLogs = 20; nit: s/th/thm/ (sorry I didn't see this on previous iteration...)
On 2015/09/21 21:38:29, wolenetz wrote: > lgtm % two tiny nits > > Thanks for fixing this! (Also, it would be good to try to repro b/24066260 to > see if the unmodified original player is fixed with this change. Results, of > course, can be reported within that b/.) strobe@ has already deployed a workaround for YouTube player (see https://b.corp.google.com/u/0/issues/24066260#comment6), so the issue doesn't repro for now on YouTube player even with very small MSE buffer sizes (1Mb audio/4Mb video) and multiple seek attemps. I'll follow up on that bug to see if that workaround should now be removed. In the meantime - how do I go with getting this fix cherry-picked into Chrome 46? > https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... > File media/filters/source_buffer_stream.cc (right): > > https://codereview.chromium.org/1347483003/diff/100001/media/filters/source_b... > media/filters/source_buffer_stream.cc:735: return bytes_freed >= bytes_to_free; > On 2015/09/21 21:09:57, servolk wrote: > > On 2015/09/21 20:14:39, wolenetz wrote: > > > nit: I recommend adding another LIMITED_MEDIA_LOG(DEBUG...) here, capped by > a > > > different counter (also with cap, maybe of just 5), for when bytes_freed < > > > bytes_to_free. > > > > Actually I think in this case we should use (D)VLOG instead of > > LIMITED_MEDIA_LOG. First of all this shouldn't happen very often in properly > > written apps and if it does happen many times (e.g. during a very long media > > playback) we want to see all instances where this has occured and now just the > > first few ones. Also just seeing this in the media log is not that useful > > without seeing the rest of the context (how much data was buffered before, > what > > exactly the GC algorithm attempted to do) which is currenty logged via DVLOG. > So > > using MEDIA_LOG here is probably not that helpful, please let me know if you > > disagree. > > Acknowledged. Except in the iteration of the stream append loop, quota exceeded > exception is also given to the app synchronously on an append's failure to evict > enough to stay under the limit, so we have quite a bit of state already > available to the developer. So existing DVLOG is ok for now. I'm strongly > considering expanding upon the decode error enumeration in the HTMLMediaElement > API so that more granular, *and*, standardized error codes for things like this > can be exposed. Of course, that's outside the scope of this CL :) > > https://codereview.chromium.org/1347483003/diff/120001/media/filters/chunk_de... > File media/filters/chunk_demuxer_unittest.cc (right): > > https://codereview.chromium.org/1347483003/diff/120001/media/filters/chunk_de... > media/filters/chunk_demuxer_unittest.cc:3468: // Append data to complete seek > operation > nit: nix this comment line since we're not seeking? > > https://codereview.chromium.org/1347483003/diff/120001/media/filters/source_b... > File media/filters/source_buffer_stream.cc (right): > > https://codereview.chromium.org/1347483003/diff/120001/media/filters/source_b... > media/filters/source_buffer_stream.cc:38: const int > kMaxGarbageCollectAlgorithWarningLogs = 20; > nit: s/th/thm/ (sorry I didn't see this on previous iteration...)
https://codereview.chromium.org/1347483003/diff/120001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1347483003/diff/120001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:3468: // Append data to complete seek operation On 2015/09/21 21:38:28, wolenetz wrote: > nit: nix this comment line since we're not seeking? Done. https://codereview.chromium.org/1347483003/diff/120001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1347483003/diff/120001/media/filters/source_b... media/filters/source_buffer_stream.cc:38: const int kMaxGarbageCollectAlgorithWarningLogs = 20; On 2015/09/21 21:38:28, wolenetz wrote: > nit: s/th/thm/ (sorry I didn't see this on previous iteration...) Done.
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/1347483003/#ps160001 (title: "Nit: removed incorrect comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1347483003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1347483003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
On 2015/09/21 22:12:32, servolk wrote: > In the meantime - how do I go with getting this fix cherry-picked into Chrome 46? The process is generally: wait for fix to land in ToT and release in at least 1 Canary or Dev official release build. Mark the bug with the additional milestone (M-47 and M-46), request merge to M-46 in the bug (I'll share further instructions in email with you), and identify any other changes in ToT that might cause merge-conflict of cherry-picking this one into M-46's branch head. Once merge is approved, there are specific steps to land and follow the builders to ensure the merge to the release branch doesn't break the build (I'll share the general info with you in email.)
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1347483003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1347483003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/1347483003/#ps180001 (title: "Rebase to ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1347483003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1347483003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1347483003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1347483003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1347483003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1347483003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e41e206081c2996cce1385efef219fa3fc0c2581 Cr-Commit-Position: refs/heads/master@{#350104} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
