Remove MediaTracks::getFirstAudio/VideoConfig

Remove MediaTracks::getFirstAudio/VideoConfig

Committed: https://crrev.com/372da91a1ab4514d35556574c7c643d47ac6f9b3
Cr-Commit-Position: refs/heads/master@{#400291}

[wolenetz, 2016-03-30 20:17:01]

wolenetz@chromium.org changed reviewers:
+ wolenetz@chromium.org

[wolenetz, 2016-03-30 20:17:02]

Looks pretty good, though the bot failures indicate there's a problem,
potentially in the prereq CL(s).

[servolk, 2016-04-07 23:20:42]

On 2016/03/30 20:17:02, wolenetz wrote:
> Looks pretty good, though the bot failures indicate there's a problem,
> potentially in the prereq CL(s).

This should be good now, the bot failures in some earlier patchsets were caused
by the other CLs that this one depends on.

[wolenetz, 2016-05-04 22:32:29]

The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run

[wolenetz, 2016-05-04 22:32:57]

On 2016/04/07 23:20:42, servolk wrote:
> On 2016/03/30 20:17:02, wolenetz wrote:
> > Looks pretty good, though the bot failures indicate there's a problem,
> > potentially in the prereq CL(s).
> 
> This should be good now, the bot failures in some earlier patchsets were
caused
> by the other CLs that this one depends on.

I'll pick this up when I'm back on Tuesday 5/10.

[commit-bot: I haz the power, 2016-05-04 22:33:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833053005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833053005/140001

[commit-bot: I haz the power, 2016-05-04 22:36:03]

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

[commit-bot: I haz the power, 2016-05-04 22:36:04]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wolenetz, 2016-06-07 20:34:21]

Note: Per servolk@, this CR currently pends (in-progress) rework and landing of
https://codereview.chromium.org/1922333002/ (and potential adjustment
afterwards).

[servolk, 2016-06-14 18:48:59]

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

[wolenetz, 2016-06-16 19:55:31]

On 2016/06/07 20:34:21, wolenetz wrote:
> Note: Per servolk@, this CR currently pends (in-progress) rework and landing
of
> https://codereview.chromium.org/1922333002/ (and potential adjustment
> afterwards).

Dependency changed from https://codereview.chromium.org/1922333002/ to
https://codereview.chromium.org/2050043002/, which landed earlier today. I'll
review this shortly.

[wolenetz, 2016-06-16 19:59:13]

The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 20:00:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833053005/200001

[wolenetz, 2016-06-16 20:20:35]

wolenetz@chromium.org changed reviewers:
- chcunningham@chromium.org

[wolenetz, 2016-06-16 20:20:36]

R-=chcunningham@ (I confirmed same with Chris first).

One comment, one nit. Looking pretty good.

https://codereview.chromium.org/1833053005/diff/200001/media/formats/mp2t/mp2...
File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1833053005/diff/200001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser_unittest.cc:127: EXPECT_TRUE(has_video_);
Versus previous code, this misses catching a potential regression where
|has_video_| is true, but no MediaTrack::Video track is in |tracks| (or perhaps
the video track has been mis-parsed as an audio track, for example).

https://codereview.chromium.org/1833053005/diff/200001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1833053005/diff/200001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:124: const auto& audio_config =
tracks->getAudioConfig(track_id);
nit: why not drop this local var and just assign to |audio_decoder_config_|
here? (and likewise for video, below)

[commit-bot: I haz the power, 2016-06-16 20:46:19]

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

[commit-bot: I haz the power, 2016-06-16 20:46:21]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-06-16 21:14:07]

https://codereview.chromium.org/1833053005/diff/200001/media/formats/mp2t/mp2...
File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1833053005/diff/200001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser_unittest.cc:127: EXPECT_TRUE(has_video_);
On 2016/06/16 20:20:36, wolenetz wrote:
> Versus previous code, this misses catching a potential regression where
> |has_video_| is true, but no MediaTrack::Video track is in |tracks| (or
perhaps
> the video track has been mis-parsed as an audio track, for example).

Yep, good catch, thanks! We should also check that at least one audio track is
always present too. I've fixed this in the latest PS.

https://codereview.chromium.org/1833053005/diff/200001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1833053005/diff/200001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:124: const auto& audio_config =
tracks->getAudioConfig(track_id);
On 2016/06/16 20:20:36, wolenetz wrote:
> nit: why not drop this local var and just assign to |audio_decoder_config_|
> here? (and likewise for video, below)

Done.

[servolk, 2016-06-16 21:14:52]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 21:15:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833053005/240001

[wolenetz, 2016-06-16 21:25:00]

lgtm % a nit I missed previously.

Thanks for doing this!

https://codereview.chromium.org/1833053005/diff/240001/media/formats/mp2t/mp2...
File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1833053005/diff/240001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser_unittest.cc:134: NOTREACHED();
Sorry I missed this before: Don't use NOTREACHED() in tests per code style:
"Don't use these macros in tests, as they crash the test binary and leave bots
in a bad state. Use the ASSERT_xx() and EXPECT_xx() family of macros, which
report failures gracefully and can continue running other tests."

Unfortunately, it looks like we already violate that in a couple other places in
this file, too. At least don't introduce another instance here please.

https://codereview.chromium.org/1833053005/diff/240001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1833053005/diff/240001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:173:
DCHECK(second_highest_timestamp != kNoDecodeTimestamp());
If you opt to fix all those NOTREACHED() in the other test file, please also
change this to an ASSERT_xx() for the same reason.

[servolk, 2016-06-16 21:33:24]

https://codereview.chromium.org/1833053005/diff/240001/media/formats/mp2t/mp2...
File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1833053005/diff/240001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser_unittest.cc:134: NOTREACHED();
On 2016/06/16 21:25:00, wolenetz wrote:
> Sorry I missed this before: Don't use NOTREACHED() in tests per code style:
> "Don't use these macros in tests, as they crash the test binary and leave bots
> in a bad state. Use the ASSERT_xx() and EXPECT_xx() family of macros, which
> report failures gracefully and can continue running other tests."
> 
> Unfortunately, it looks like we already violate that in a couple other places
in
> this file, too. At least don't introduce another instance here please.

Oh, good to know, thanks, I'll fix this.

https://codereview.chromium.org/1833053005/diff/240001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1833053005/diff/240001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:173:
DCHECK(second_highest_timestamp != kNoDecodeTimestamp());
On 2016/06/16 21:25:00, wolenetz wrote:
> If you opt to fix all those NOTREACHED() in the other test file, please also
> change this to an ASSERT_xx() for the same reason.

Done.

[wolenetz, 2016-06-16 22:19:30]

lgtm Thanks again.

[servolk, 2016-06-16 22:21:18]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-06-16 22:22:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833053005/260001

[commit-bot: I haz the power, 2016-06-16 23:26:45]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-06-16 23:29:36]

Description was changed from

==========
Remove MediaTracks::getFirstAudio/VideoConfig
==========

to

==========
Remove MediaTracks::getFirstAudio/VideoConfig

Committed: https://crrev.com/372da91a1ab4514d35556574c7c643d47ac6f9b3
Cr-Commit-Position: refs/heads/master@{#400291}
==========

[commit-bot: I haz the power, 2016-06-16 23:29:37]

Patchset 14 (id:??) landed as
https://crrev.com/372da91a1ab4514d35556574c7c643d47ac6f9b3
Cr-Commit-Position: refs/heads/master@{#400291}