Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2110853006:
Fix MultibufferDataSource::GetSize (Closed)
DescriptionFix MultibufferDataSource::GetSize
When file loading is finished ResourceMultiBufferDataProvider count
DataBuffer queue size twice, so GetSize returned incorrect value.
BUG=
Committed: https://crrev.com/bbaba3c8053a39f6d11d78a6e4770b37a9639067
Cr-Commit-Position: refs/heads/master@{#403123}
Patch Set 1 #
Messages
Total messages: 21 (6 generated)
Description was changed from ========== Fix MultibufferDataSource::GetSize When file loading is finished ResourceMultiBufferDataProvider count DataBuffer queue size twice, so GetSize returned incorrect value. BUG= ========== to ========== Fix MultibufferDataSource::GetSize When file loading is finished ResourceMultiBufferDataProvider count DataBuffer queue size twice, so GetSize returned incorrect value. BUG= ==========
kostya-k@yandex-team.ru changed reviewers: + hubbe@chromium.org
LGTM Is there a BUG= for this?
On 2016/06/30 06:23:26, hubbe wrote: > LGTM > > Is there a BUG= for this? I didn't find any.
The CQ bit was checked by kostya-k@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix MultibufferDataSource::GetSize When file loading is finished ResourceMultiBufferDataProvider count DataBuffer queue size twice, so GetSize returned incorrect value. BUG= ========== to ========== Fix MultibufferDataSource::GetSize When file loading is finished ResourceMultiBufferDataProvider count DataBuffer queue size twice, so GetSize returned incorrect value. BUG= ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Fix MultibufferDataSource::GetSize When file loading is finished ResourceMultiBufferDataProvider count DataBuffer queue size twice, so GetSize returned incorrect value. BUG= ========== to ========== Fix MultibufferDataSource::GetSize When file loading is finished ResourceMultiBufferDataProvider count DataBuffer queue size twice, so GetSize returned incorrect value. BUG= Committed: https://crrev.com/bbaba3c8053a39f6d11d78a6e4770b37a9639067 Cr-Commit-Position: refs/heads/master@{#403123} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/bbaba3c8053a39f6d11d78a6e4770b37a9639067 Cr-Commit-Position: refs/heads/master@{#403123}
Message was sent while issue was closed.
On 2016/06/30 08:53:35, commit-bot: I haz the power wrote: > Patchset 1 (id:??) landed as > https://crrev.com/bbaba3c8053a39f6d11d78a6e4770b37a9639067 > Cr-Commit-Position: refs/heads/master@{#403123}
Message was sent while issue was closed.
On 2016/06/30 07:08:04, kostya-k wrote: > On 2016/06/30 06:23:26, hubbe wrote: > > LGTM > > > > Is there a BUG= for this? > > I didn't find any. So what side effects does this bug have? How did you discover it?
Message was sent while issue was closed.
On 2016/06/30 17:22:50, hubbe wrote: > On 2016/06/30 07:08:04, kostya-k wrote: > > On 2016/06/30 06:23:26, hubbe wrote: > > > LGTM > > > > > > Is there a BUG= for this? > > > > I didn't find any. > > So what side effects does this bug have? How did you discover it? I believe this CL is causing flakyness in the video related tests on the GPU FYI waterfall. Examples: https://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Release%20%28NV... https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20Retina%20Release... Seeing this in the output log: [27562:1287:0630/083352:WARNING:webmediaplayer_impl.cc(335)] Using MultibufferDataSource [27562:34423:0630/083352:ERROR:ffmpeg_demuxer.cc(1510)] OnReadFrameDone result=-541478725 IsMaxMemoryUsageReached=0 [27562:1287:0630/083352:WARNING:webmediaplayer_impl.cc(335)] Using MultibufferDataSource [27562:1287:0630/083352:FATAL:resource_multibuffer_data_provider.cc(75)] Check failed: byte_pos() < url_data_->length() (32768 vs. 10292) http://127.0.0.1:53903/resources/red-green.theora.ogv
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2115483003/ by geofflang@chromium.org. The reason for reverting is: I believe this CL is causing flakyness in the video related tests on the GPU FYI waterfall. Examples: https://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Release%20%28NV... https://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Release%20%28A... https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20Retina%20Release... Seeing this in the output log: [27562:1287:0630/083352:WARNING:webmediaplayer_impl.cc(335)] Using MultibufferDataSource [27562:34423:0630/083352:ERROR:ffmpeg_demuxer.cc(1510)] OnReadFrameDone result=-541478725 IsMaxMemoryUsageReached=0 [27562:1287:0630/083352:WARNING:webmediaplayer_impl.cc(335)] Using MultibufferDataSource [27562:1287:0630/083352:FATAL:resource_multibuffer_data_provider.cc(75)] Check failed: byte_pos() < url_data_->length() (32768 vs. 10292) http://127.0.0.1:53903/resources/red-green.theora.ogv.
Message was sent while issue was closed.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Message was sent while issue was closed.
When relanding this (if you do), please create a bug so we can get this merged to m52.
Message was sent while issue was closed.
On 2016/06/30 17:22:50, hubbe wrote: > On 2016/06/30 07:08:04, kostya-k wrote: > > On 2016/06/30 06:23:26, hubbe wrote: > > > LGTM > > > > > > Is there a BUG= for this? > > > > I didn't find any. > > So what side effects does this bug have? How did you discover it? Well, we have a peculiar media pipeline in Yandex browser that uses kind of a proxy DataSource which takes another DataSource and tries to fetch all requested data from it. For example when we try to play bear_silent.mp4 (which is exacly 30451 bytes long) DataSource::GetSize returns 32k because of this bug. Then client asks to read 32k of data and proxy DataSource at first receives 30451 bytes and it tries to fetch remaining data, but it fails due to there are no more data. I haven't really thought that it will cause test flakyness because it's a bug for sure. As you can see fifo_.back() size is already counted in byte_pos() https://cs.chromium.org/chromium/src/media/blink/resource_multibuffer_data_pr... And BufferedDataSource always returns correct size
Message was sent while issue was closed.
On 2016/07/01 03:18:57, kostya-k wrote: > On 2016/06/30 17:22:50, hubbe wrote: > > On 2016/06/30 07:08:04, kostya-k wrote: > > > On 2016/06/30 06:23:26, hubbe wrote: > > > > LGTM > > > > > > > > Is there a BUG= for this? > > > > > > I didn't find any. > > > > So what side effects does this bug have? How did you discover it? > > Well, we have a peculiar media pipeline in Yandex browser that uses kind of a > proxy DataSource which takes another DataSource and tries to fetch all requested > data from it. For example when we try to play bear_silent.mp4 (which is exacly > 30451 bytes long) DataSource::GetSize returns 32k because of this bug. Then > client asks to read 32k of data and proxy DataSource at first receives 30451 > bytes and it tries to fetch remaining data, but it fails due to there are no > more data. > I haven't really thought that it will cause test flakyness because it's a bug > for sure. As you can see fifo_.back() size is already counted in byte_pos() > https://cs.chromium.org/chromium/src/media/blink/resource_multibuffer_data_pr... > And BufferedDataSource always returns correct size I agree that it's a bug, but it looks like fixing it breaks something else right now, which also needs to be fixed. However, it's important to file a bugreport to document the impact of a particular bug, as that will help us decide if the bug needs to be merged back to older branches, and also helps us triage similar failures.
Message was sent while issue was closed.
On 2016/07/01 17:12:24, hubbe wrote: > On 2016/07/01 03:18:57, kostya-k wrote: > > On 2016/06/30 17:22:50, hubbe wrote: > > > On 2016/06/30 07:08:04, kostya-k wrote: > > > > On 2016/06/30 06:23:26, hubbe wrote: > > > > > LGTM > > > > > > > > > > Is there a BUG= for this? > > > > > > > > I didn't find any. > > > > > > So what side effects does this bug have? How did you discover it? > > > > Well, we have a peculiar media pipeline in Yandex browser that uses kind of a > > proxy DataSource which takes another DataSource and tries to fetch all > requested > > data from it. For example when we try to play bear_silent.mp4 (which is exacly > > 30451 bytes long) DataSource::GetSize returns 32k because of this bug. Then > > client asks to read 32k of data and proxy DataSource at first receives 30451 > > bytes and it tries to fetch remaining data, but it fails due to there are no > > more data. > > I haven't really thought that it will cause test flakyness because it's a bug > > for sure. As you can see fifo_.back() size is already counted in byte_pos() > > > https://cs.chromium.org/chromium/src/media/blink/resource_multibuffer_data_pr... > > And BufferedDataSource always returns correct size > > I agree that it's a bug, but it looks like fixing it breaks something else right > now, which also needs to be fixed. > However, it's important to file a bugreport to document the impact of a > particular bug, as that will help us decide > if the bug needs to be merged back to older branches, and also helps us triage > similar failures. Yup, here it is - https://bugs.chromium.org/p/chromium/issues/detail?id=625515 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
