Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
51983002:
Update selected range after truncation (Closed)
|
Created:
7 years, 1 month ago by Jinsuk Kim Modified:
7 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUpdate selected range after truncation
Update |SourceBufferStream::selected_range_| after |TruncateAt| is called in case the range pointed to by it gets deleted.
There are situations where next_buffer_index_ remains at -1
where|ChunkDemuxer::AppendData| is invoked. This eventually
causes crash. This changes prevents such cases.
The crash is occasionally observed when YouTube MSE/EME
conformance test http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/tot.html
(41.MediaSourceDuration) is done on Android TV device.
BUG=313102
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233468
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : addressed comments #Patch Set 4 : Added unittest #
Total comments: 8
Patch Set 5 : addressed comments #
Total comments: 6
Patch Set 6 : addressed comments #Messages
Total messages: 17 (0 generated)
acolwell: can you review this? jindor: do we have a bug filed?
On 2013/10/30 04:13:23, scherkus wrote: > acolwell: can you review this? > > jindor: do we have a bug filed? Just filed one. crbug/313102
On 2013/10/30 04:34:22, jindor wrote: > On 2013/10/30 04:13:23, scherkus wrote: > > acolwell: can you review this? > > > > jindor: do we have a bug filed? > > Just filed one. crbug/313102 @jindor, IMO, we need to identify the root cause more deeply.
On 2013/10/30 11:22:19, Yuncheol Heo wrote: > On 2013/10/30 04:34:22, jindor wrote: > > On 2013/10/30 04:13:23, scherkus wrote: > > > acolwell: can you review this? > > > > > > jindor: do we have a bug filed? > > > > Just filed one. crbug/313102 > > @jindor, > IMO, we need to identify the root cause more deeply. This doesn't seem right. If the description is correct, then the DCHECK(HasNextBufferPosition()) at the top of the method should be firing. This likely means the method is being called in an unexpected context. You should dig deeper to figure out why and then add a unit test to source_buffer_stream_unittests.cc that reproduces the crash situation and verifies the fix.
On 2013/10/30 15:17:31, acolwell wrote: > On 2013/10/30 11:22:19, Yuncheol Heo wrote: > > On 2013/10/30 04:34:22, jindor wrote: > > > On 2013/10/30 04:13:23, scherkus wrote: > > > > acolwell: can you review this? > > > > > > > > jindor: do we have a bug filed? > > > > > > Just filed one. crbug/313102 > > > > @jindor, > > IMO, we need to identify the root cause more deeply. > > This doesn't seem right. If the description is correct, then the > DCHECK(HasNextBufferPosition()) at the top of the method should be firing. This > likely means the method is being called in an unexpected context. You should dig > deeper to figure out why and then add a unit test to > source_buffer_stream_unittests.cc that reproduces the crash situation and > verifies the fix. Agree it was more of a quick-and-dirty fix... Digged further to figure out what's really going wrong under the hood. It turns out |TruncateAt| called by |OnSetDuration| reset next_buffer_index_ to -1, thus selected_range_ got invalidated. But it was not properly updated to NULL, which I think is a bug. The updated change rectifies it. Let me know if the new change looks fine. I'll add a safeguard test for it.
https://codereview.chromium.org/51983002/diff/80001/media/filters/source_buff... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/51983002/diff/80001/media/filters/source_buff... media/filters/source_buffer_stream.cc:1071: if (selected_range_ && !selected_range_->HasNextBufferPosition()) nit: s/selected_range_ &&/(*itr == selected_range_) &&/ just so this is more explicit. https://codereview.chromium.org/51983002/diff/80001/media/filters/source_buff... media/filters/source_buffer_stream.cc:1840: if (next_buffer_index_ >= static_cast<int>(buffers_.size())) { With the change above do you really need this change?
https://codereview.chromium.org/51983002/diff/80001/media/filters/source_buff... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/51983002/diff/80001/media/filters/source_buff... media/filters/source_buffer_stream.cc:1071: if (selected_range_ && !selected_range_->HasNextBufferPosition()) On 2013/11/04 20:58:11, acolwell wrote: > nit: s/selected_range_ &&/(*itr == selected_range_) &&/ just so this is more > explicit. Done. https://codereview.chromium.org/51983002/diff/80001/media/filters/source_buff... media/filters/source_buffer_stream.cc:1840: if (next_buffer_index_ >= static_cast<int>(buffers_.size())) { On 2013/11/04 20:58:11, acolwell wrote: > With the change above do you really need this change? Mine was already reverted. This code was already here.
This looks fine. Please add a test to SourceBufferStreamTest that exposes the bug in the old code and verifies the fix.
On 2013/11/05 04:27:08, acolwell wrote: > This looks fine. Please add a test to SourceBufferStreamTest that exposes the > bug in the old code and verifies the fix. Added a test. Please take another look. Thanks. One thing to note in the way the test is written is, in order to simulate the situation in interest I needed to access a stream parser buffer inside SourceBufferRange to modify the decode timestamp. I found, however, I cannot access SourceBufferRange without refactoring source_buffer_stream.cc and exposing the class which is used only internally. Rather than modifying the main class, I chose to create a member variable (queue_) in the test class to get the direct access the parser buffer.
https://codereview.chromium.org/51983002/diff/180001/media/filters/source_buf... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/51983002/diff/180001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2890: SetBufferDecodeTimestamp(base::TimeDelta::FromMilliseconds(99)); Can you use 'CheckExpectedBuffers(0, 5)' to move the next buffer index?
https://codereview.chromium.org/51983002/diff/180001/media/filters/source_buf... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/51983002/diff/180001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2883: NewSegmentAppend(0, 5); For new tests we prefer to use the string version of these methods so that it is easier to see what is going on. See the test below this one for an example. https://codereview.chromium.org/51983002/diff/180001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2890: SetBufferDecodeTimestamp(base::TimeDelta::FromMilliseconds(99)); This method is doing an unnatural transformation on the buffers that cannot be triggered from JavaScript. Please rework this so it reflects what can actually be triggered by JS. https://codereview.chromium.org/51983002/diff/180001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2898: ASSERT_EQ(kNoTimestamp(), GetNextBufferTimestamp()); I think you should use CheckExpectedRangesByTimestamp(), CheckExpectedBuffers(), and/or CheckNoNextBuffer() to verify the desired state here. The timestamp isn't really the important part for the external code.
Thanks for the review. Please take another look. https://codereview.chromium.org/51983002/diff/180001/media/filters/source_buf... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/51983002/diff/180001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2883: NewSegmentAppend(0, 5); On 2013/11/05 19:12:58, acolwell wrote: > For new tests we prefer to use the string version of these methods so that it is > easier to see what is going on. See the test below this one for an example. Done. https://codereview.chromium.org/51983002/diff/180001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2890: SetBufferDecodeTimestamp(base::TimeDelta::FromMilliseconds(99)); On 2013/11/05 10:32:42, Yuncheol Heo wrote: > Can you use 'CheckExpectedBuffers(0, 5)' to move the next buffer index? I used GetNextBuffer() instead since CheckExpectedBuffers not only move the next buffer but also checks the current buffer status with the given parameter, and possibly marks the test failure. https://codereview.chromium.org/51983002/diff/180001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2890: SetBufferDecodeTimestamp(base::TimeDelta::FromMilliseconds(99)); On 2013/11/05 19:12:58, acolwell wrote: > This method is doing an unnatural transformation on the buffers that cannot be > triggered from JavaScript. Please rework this so it reflects what can actually > be triggered by JS. Agreed. See if the change addresses your comment. https://codereview.chromium.org/51983002/diff/180001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2898: ASSERT_EQ(kNoTimestamp(), GetNextBufferTimestamp()); On 2013/11/05 19:12:58, acolwell wrote: > I think you should use CheckExpectedRangesByTimestamp(), CheckExpectedBuffers(), > and/or CheckNoNextBuffer() to verify the desired state here. The timestamp isn't > really the important part for the external code. Done. Used CheckExpectedRangesByTimestamp.
lgtm % nits https://codereview.chromium.org/51983002/diff/250001/media/filters/source_buf... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/51983002/diff/250001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2878: scoped_refptr<StreamParserBuffer> buffer; nit: Replace these three lines with CheckExpectedBuffers("0K 30"); to achieve the same thing and match the style of other tests. https://codereview.chromium.org/51983002/diff/250001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2883: stream_->OnSetDuration(frame_duration() * 1); nit: Please use base::TimeDelta::FromMilliseconds(60) here. I think that will make it easier to follow what is happening and not lead to confusion about why 60 appears in the ranges below. It tests the same code paths. https://codereview.chromium.org/51983002/diff/250001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2884: nit: Add CheckNoNextBuffer() here to verify that there is no next buffer.
https://codereview.chromium.org/51983002/diff/250001/media/filters/source_buf... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/51983002/diff/250001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2878: scoped_refptr<StreamParserBuffer> buffer; On 2013/11/06 05:26:00, acolwell wrote: > nit: Replace these three lines with CheckExpectedBuffers("0K 30"); to achieve > the same thing and match the style of other tests. Done. ycheo@: This is what you suggested. Maybe I was wrong. Thanks for the suggestion. https://codereview.chromium.org/51983002/diff/250001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2883: stream_->OnSetDuration(frame_duration() * 1); On 2013/11/06 05:26:00, acolwell wrote: > nit: Please use base::TimeDelta::FromMilliseconds(60) here. I think that will > make it easier to follow what is happening and not lead to confusion about why > 60 appears in the ranges below. It tests the same code paths. Done. https://codereview.chromium.org/51983002/diff/250001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:2884: On 2013/11/06 05:26:00, acolwell wrote: > nit: Add CheckNoNextBuffer() here to verify that there is no next buffer. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jinsukkim@chromium.org/51983002/300001
Message was sent while issue was closed.
Change committed as 233468 |
