Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(61)

Issue 379693002: Update SourceBufferStream and its unit tests to always expect valid durations. (Closed)

Created:
6 years, 5 months ago by acolwell GONE FROM CHROMIUM
Modified:
6 years, 5 months ago
Reviewers:
DaleCurtis, wolenetz
CC:
chromium-reviews, feature-media-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Update SourceBufferStream and its unit tests to always expect valid durations. This change fixes the SourceBufferStream unit tests so that they always provide valid durations for buffers that it passes to SourceBufferStream. I've also added code to SoureBufferStream to verify that it always gets buffers with valid durations. Minor tweaks to test expectations were needed to compensate for the SourceBufferStream behaving differently when it got actual durations instead of using the durations it made up. In most cases I just used the duration the SourceBufferStream was ultimately using. In a few cases the duration the SourceBufferStream was generating didn't make any sense so I simply changed the expectations to match the new behavior. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283365

Patch Set 1 #

Total comments: 54

Patch Set 2 : Rebase and address CR comments #

Total comments: 2

Patch Set 3 : Address CR comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+111 lines, -71 lines) Patch
M media/base/decoder_buffer.h View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M media/base/stream_parser_buffer.cc View 1 2 chunks +7 lines, -0 lines 0 comments Download
M media/filters/source_buffer_stream.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M media/filters/source_buffer_stream_unittest.cc View 1 39 chunks +95 lines, -71 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
acolwell GONE FROM CHROMIUM
6 years, 5 months ago (2014-07-09 02:01:12 UTC) #1
acolwell GONE FROM CHROMIUM
https://codereview.chromium.org/379693002/diff/1/media/filters/source_buffer_stream_unittest.cc File media/filters/source_buffer_stream_unittest.cc (left): https://codereview.chromium.org/379693002/diff/1/media/filters/source_buffer_stream_unittest.cc#oldcode1597 media/filters/source_buffer_stream_unittest.cc:1597: CheckExpectedBuffers("70 100K 120K 130K"); I haven't 100% convinced myself ...
6 years, 5 months ago (2014-07-09 02:07:02 UTC) #2
wolenetz
First round of comments/nits/questions: https://codereview.chromium.org/379693002/diff/1/media/base/decoder_buffer.h File media/base/decoder_buffer.h (right): https://codereview.chromium.org/379693002/diff/1/media/base/decoder_buffer.h#newcode82 media/base/decoder_buffer.h:82: << duration.InSecondsF(); nit: disallow kInfiniteDuration() ...
6 years, 5 months ago (2014-07-09 22:42:18 UTC) #3
acolwell GONE FROM CHROMIUM
https://codereview.chromium.org/379693002/diff/1/media/base/decoder_buffer.h File media/base/decoder_buffer.h (right): https://codereview.chromium.org/379693002/diff/1/media/base/decoder_buffer.h#newcode82 media/base/decoder_buffer.h:82: << duration.InSecondsF(); On 2014/07/09 22:42:16, wolenetz wrote: > nit: ...
6 years, 5 months ago (2014-07-15 18:49:28 UTC) #4
wolenetz
lgtm % one nit https://codereview.chromium.org/379693002/diff/1/media/filters/source_buffer_stream_unittest.cc File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/379693002/diff/1/media/filters/source_buffer_stream_unittest.cc#newcode279 media/filters/source_buffer_stream_unittest.cc:279: if (timestamps[i] == "C") On ...
6 years, 5 months ago (2014-07-15 21:05:38 UTC) #5
DaleCurtis
https://codereview.chromium.org/379693002/diff/1/media/filters/source_buffer_stream_unittest.cc File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/379693002/diff/1/media/filters/source_buffer_stream_unittest.cc#newcode542 media/filters/source_buffer_stream_unittest.cc:542: buffer->ConvertToSpliceBuffer(pre_splice_buffers); On 2014/07/15 18:49:27, acolwell wrote: > On 2014/07/09 ...
6 years, 5 months ago (2014-07-15 21:06:41 UTC) #6
acolwell GONE FROM CHROMIUM
https://codereview.chromium.org/379693002/diff/20001/media/base/decoder_buffer.h File media/base/decoder_buffer.h (right): https://codereview.chromium.org/379693002/diff/20001/media/base/decoder_buffer.h#newcode82 media/base/decoder_buffer.h:82: duration == kInfiniteDuration()) On 2014/07/15 21:05:38, wolenetz wrote: > ...
6 years, 5 months ago (2014-07-15 21:19:37 UTC) #7
acolwell GONE FROM CHROMIUM
The CQ bit was checked by acolwell@chromium.org
6 years, 5 months ago (2014-07-15 21:19:43 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/379693002/40001
6 years, 5 months ago (2014-07-15 21:23:18 UTC) #9
commit-bot: I haz the power
Change committed as 283365
6 years, 5 months ago (2014-07-16 05:47:46 UTC) #10
falken
6 years, 5 months ago (2014-07-16 08:55:12 UTC) #11
Message was sent while issue was closed.
A revert of this CL has been created in
https://codereview.chromium.org/392193003/ by falken@chromium.org.

The reason for reverting is: This seems to have broken several layout tests:

event-attributes.html 
media-controller-time-clamp.html 
video-currentTime-delay.html 
video-currentTime-set.html 
video-duration-known-after-eos.html 
video-loop.html 
video-playbackrate.html 
video-played-collapse.html 
video-seek-past-end-paused.html 
video-seek-past-end-playing.html 
video-seek-to-duration-with-playbackrate-zero.html 

See for example this build:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6%20(dbg)....

Powered by Google App Engine
This is Rietveld 408576698