Roll ffmpeg DEPS for MP3 seek bug, add tests.

Roll ffmpeg DEPS for MP3 seek bug, add tests.

Summary of changes available at:
https://chromium.googlesource.com/chromium/third_party/ffmpeg/+log/ddfb37c..b5b1d0e

BUG=545914
R=ddorwin@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/33c32611cb9a36eacae01b336e3c717baf4d53a8

[chcunningham, 2015-12-03 00:51:35]

Description was changed from

==========
Roll ffmpeg DEPS for MP3 seek bug, add tests.

BUG=545914
==========

to

==========
Roll ffmpeg DEPS for MP3 seek bug, add tests.

Summary of changes available at:
https://chromium.googlesource.com/chromium/third_party/ffmpeg/+log/ddfb37c..b...

BUG=545914
==========

[chcunningham, 2015-12-03 00:51:35]

chcunningham@chromium.org changed reviewers:
+ ddorwin@chromium.org

[chcunningham, 2015-12-03 00:58:48]

DEPS and regression tests for the mp3 bug.

[ddorwin, 2015-12-03 23:07:54]

LG. Minor stuff and nits.

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1239: class
Mp3SeekPipelineIntegrationTest
nit: Why now empty lines around this class?

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1242:
TEST_P(Mp3SeekPipelineIntegrationTest, FastSeekAccuracy_MP3) {
Should we have such tests for other types?

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1244: ASSERT_EQ(PIPELINE_OK,
Start(config.filename, kHashed));
None of the ASSERTed items will cause a crash. Generally prefer EXPECT_.

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1248: // NOTE this seek time of (.3 *
duration) is deliberately chosen to expose
nit: 0.3? Also, below.

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1251: // - Each byte is read as a uint8,
value between 0 - 255.
nit: remove the comma or replace it with something else

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1258: // (.3 * duration) should be
around (.3 * file size). This is 78.8 / 256,
7_6_.8?

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1259: // but the numerator will be
truncated in the TOC as 78, loosing precision.
76?

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1261: // TLDR:
This comment block should probably be external to the test code. Also, this
seems like the introduction followed by the Quick TOC design followed by
discussion of .3. WDYT?

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1263: // faster seeking, see
Mp3SeekFFmpegDemuxerTest). The chosen seek time
nit: semicolon instead of colon? This isn't intended to be a preposition is it?

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1271:
ASSERT_TRUE(WaitUntilCurrentTimeIsAfter(end_time));
Is this call somehow frame-accurate? Otherwise, the hash would vary, right? How
does it work?

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1277: ,
?

[chcunningham, 2015-12-04 02:29:55]

Thanks David

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1239: class
Mp3SeekPipelineIntegrationTest
On 2015/12/03 23:07:54, ddorwin wrote:
> nit: Why now empty lines around this class?

Fixed

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1242:
TEST_P(Mp3SeekPipelineIntegrationTest, FastSeekAccuracy_MP3) {
On 2015/12/03 23:07:54, ddorwin wrote:
> Should we have such tests for other types?

No, this fastseek thing is mp3 specific. See
https://docs.google.com/document/d/1jNzeyaHz3_Hkdm7Z1rS6mVG4pV4Fpq5IxLOGJS7cc...

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1244: ASSERT_EQ(PIPELINE_OK,
Start(config.filename, kHashed));
On 2015/12/03 23:07:54, ddorwin wrote:
> None of the ASSERTed items will cause a crash. Generally prefer EXPECT_.

Leaving to match the other tests in this file

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1248: // NOTE this seek time of (.3 *
duration) is deliberately chosen to expose
On 2015/12/03 23:07:53, ddorwin wrote:
> nit: 0.3? Also, below.

Done.

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1251: // - Each byte is read as a uint8,
value between 0 - 255.
On 2015/12/03 23:07:54, ddorwin wrote:
> nit: remove the comma or replace it with something else

Done.

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1258: // (.3 * duration) should be
around (.3 * file size). This is 78.8 / 256,
On 2015/12/03 23:07:54, ddorwin wrote:
> 7_6_.8?

Done.

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1259: // but the numerator will be
truncated in the TOC as 78, loosing precision.
On 2015/12/03 23:07:53, ddorwin wrote:
> 76?

Done.

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1261: // TLDR:
On 2015/12/03 23:07:53, ddorwin wrote:
> This comment block should probably be external to the test code. Also, this
> seems like the introduction followed by the Quick TOC design followed by
> discussion of .3. WDYT?

Reordered and condensed

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1263: // faster seeking, see
Mp3SeekFFmpegDemuxerTest). The chosen seek time
On 2015/12/03 23:07:54, ddorwin wrote:
> nit: semicolon instead of colon? This isn't intended to be a preposition is
it?

Done.

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1271:
ASSERT_TRUE(WaitUntilCurrentTimeIsAfter(end_time));
On 2015/12/03 23:07:54, ddorwin wrote:
> Is this call somehow frame-accurate? Otherwise, the hash would vary, right?
How
> does it work?

Great catch! reworked the test to play from near-end -> end.

https://codereview.chromium.org/1492163002/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1277: ,
On 2015/12/03 23:07:54, ddorwin wrote:
> ?

Wasn't using this, but I broke up the tests now into the "good" accurate and
"bad" inaccurate seeks

[ddorwin, 2015-12-04 19:31:50]

LGTM with comment

https://codereview.chromium.org/1492163002/diff/80001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1492163002/diff/80001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1275: AccurateSeeks,
CBRSeeks? The test itself is not accurate/inaccurate. That's just the impl.

[chcunningham, 2015-12-04 22:34:29]

https://codereview.chromium.org/1492163002/diff/80001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1492163002/diff/80001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1275: AccurateSeeks,
On 2015/12/04 19:31:50, ddorwin wrote:
> CBRSeeks? The test itself is not accurate/inaccurate. That's just the impl.

Done.

[chcunningham, 2015-12-04 22:34:34]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2015-12-04 22:34:34]

The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1492163002/#ps100001
(title: "Test name nit.")

[commit-bot: I haz the power, 2015-12-04 22:48:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492163002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492163002/100001

[commit-bot: I haz the power, 2015-12-05 00:41:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-05 00:41:16]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[commit-bot: I haz the power, 2015-12-05 02:29:09]

Description was changed from

==========
Roll ffmpeg DEPS for MP3 seek bug, add tests.

Summary of changes available at:
https://chromium.googlesource.com/chromium/third_party/ffmpeg/+log/ddfb37c..b...

BUG=545914
==========

to

==========
Roll ffmpeg DEPS for MP3 seek bug, add tests.

Summary of changes available at:
https://chromium.googlesource.com/chromium/third_party/ffmpeg/+log/ddfb37c..b...

BUG=545914
R=ddorwin@chromium.org

Committed: https://crrev.com/33c32611cb9a36eacae01b336e3c717baf4d53a8
Cr-Commit-Position: refs/heads/master@{#363330}
==========

[commit-bot: I haz the power, 2015-12-05 02:29:09]

Patchset 6 (id:??) landed as
https://crrev.com/33c32611cb9a36eacae01b336e3c717baf4d53a8
Cr-Commit-Position: refs/heads/master@{#363330}

[chcunningham, 2015-12-05 02:29:36]

Description was changed from

==========
Roll ffmpeg DEPS for MP3 seek bug, add tests.

Summary of changes available at:
https://chromium.googlesource.com/chromium/third_party/ffmpeg/+log/ddfb37c..b...

BUG=545914
R=ddorwin@chromium.org

Committed: https://crrev.com/33c32611cb9a36eacae01b336e3c717baf4d53a8
Cr-Commit-Position: refs/heads/master@{#363330}
==========

to

==========
Roll ffmpeg DEPS for MP3 seek bug, add tests.

Summary of changes available at:
https://chromium.googlesource.com/chromium/third_party/ffmpeg/+log/ddfb37c..b...

BUG=545914
R=ddorwin@chromium.org

Committed:
https://chromium.googlesource.com/chromium/src/+/33c32611cb9a36eacae01b336e3c...
==========

[chcunningham, 2015-12-05 02:29:42]

Committed patchset #6 (id:100001) manually as
33c32611cb9a36eacae01b336e3c717baf4d53a8 (presubmit successful).