Convert enable_mpeg2ts into a new-style buildflag

Convert enable_mpeg2ts_parser GYP flag into a new-style buildflag

According to recommendation given at
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5c4ySpPsV14

BUG=none

Committed: https://crrev.com/2a41135331b37b94b15df5af641421cf407b2d11
Cr-Commit-Position: refs/heads/master@{#368001}

[servolk, 2015-12-21 17:53:09]

Description was changed from

==========
Convert enable_mpeg2ts into a new-style buildflag

BUG=none
==========

to

==========
Convert enable_mpeg2ts_parser GYP flag into a new-style buildflag

According to recommendation given at
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5c4ySpPsV14

BUG=none
==========

[servolk, 2015-12-21 17:53:10]

servolk@chromium.org changed reviewers:
+ brettw@chromium.org, ddorwin@chromium.org, jochen@chromium.org

[brettw, 2016-01-04 22:06:43]

LGTM, thanks!

[brettw, 2016-01-04 22:06:52]

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

[commit-bot: I haz the power, 2016-01-04 22:07:12]

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

[commit-bot: I haz the power, 2016-01-04 22:18:57]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ddorwin, 2016-01-04 22:19:45]

LG overall. Some questions.

https://codereview.chromium.org/1539983004/diff/60001/content/browser/media/m...
File content/browser/media/media_source_browsertest.cc (right):

https://codereview.chromium.org/1539983004/diff/60001/content/browser/media/m...
content/browser/media/media_source_browsertest.cc:24: #if
defined(USE_PROPRIETARY_CODECS)
Should we convert all such media-related checks in a separate (set of) CL(s)?
Consistency will help people remember the correct way and hopefully avoid
mistakes.

https://codereview.chromium.org/1539983004/diff/60001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1539983004/diff/60001/media/BUILD.gn#newcode17
media/BUILD.gn:17: # Enables the MPEG2-TS stream parser for use with Media
Source. Disabled by
This is consistent with the previous name but now more explicit. Are there no
plans to support .src=HEVC?

https://codereview.chromium.org/1539983004/diff/60001/media/BUILD.gn#newcode18
media/BUILD.gn:18: # default (except on Chromecast) since it's not available on
the normal Web
Just keep it simple: Only supported on Chromecast.
Or just drop it since we can read the line below.

https://codereview.chromium.org/1539983004/diff/60001/media/BUILD.gn#newcode495
media/BUILD.gn:495: "formats/mp2t/es_adapter_video.cc",
Shouldn't all of these files be protected by enable_mse_mpeg2ts_stream_parser?
That's not specifically related to this CL, but I was surprised that no source
files were protected by enable_mse_mpeg2ts_stream_parser.

https://codereview.chromium.org/1539983004/diff/60001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1539983004/diff/60001/media/media.gyp#newcode...
media/media.gyp:1063: 'formats/mp2t/es_adapter_video.cc',
Ditto.

[servolk, 2016-01-05 00:18:10]

https://codereview.chromium.org/1539983004/diff/60001/content/browser/media/m...
File content/browser/media/media_source_browsertest.cc (right):

https://codereview.chromium.org/1539983004/diff/60001/content/browser/media/m...
content/browser/media/media_source_browsertest.cc:24: #if
defined(USE_PROPRIETARY_CODECS)
On 2016/01/04 22:19:45, ddorwin wrote:
> Should we convert all such media-related checks in a separate (set of) CL(s)?
> Consistency will help people remember the correct way and hopefully avoid
> mistakes.

Yes, I guess in the future we'll convert all the legacy flags into new-style
buildflags, but I think perhaps it would be better to convert each flag in a
separate CLs (or maybe each set of related flags in a separate CL). That would
make it easier to review and easier to make it work as a sequence of incremental
steps (and hopefully easier to detect and isolate regressions, in case some
conversions accidentally introduce bugs). I've decided to start with
enable_mpeg2ts since it's fairly different from all other existing media flags
(currently defined globally in common.gypi, which we want to avoid going
forward), plus mpeg2ts is only enabled by Chromecast, so this should be the
least risky flag to convert from Chromium point of view.

https://codereview.chromium.org/1539983004/diff/60001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1539983004/diff/60001/media/BUILD.gn#newcode17
media/BUILD.gn:17: # Enables the MPEG2-TS stream parser for use with Media
Source. Disabled by
On 2016/01/04 22:19:45, ddorwin wrote:
> This is consistent with the previous name but now more explicit. Are there no
> plans to support .src=HEVC?

Did you mean .src=foo.ts (mpeg2ts)? AFAIK there's no short-term plans to support
that, since almost all Chromecast apps use MPL player that works through MSE.

https://codereview.chromium.org/1539983004/diff/60001/media/BUILD.gn#newcode18
media/BUILD.gn:18: # default (except on Chromecast) since it's not available on
the normal Web
On 2016/01/04 22:19:45, ddorwin wrote:
> Just keep it simple: Only supported on Chromecast.
> Or just drop it since we can read the line below.

Done.

https://codereview.chromium.org/1539983004/diff/60001/media/BUILD.gn#newcode495
media/BUILD.gn:495: "formats/mp2t/es_adapter_video.cc",
On 2016/01/04 22:19:45, ddorwin wrote:
> Shouldn't all of these files be protected by enable_mse_mpeg2ts_stream_parser?
> That's not specifically related to this CL, but I was surprised that no source
> files were protected by enable_mse_mpeg2ts_stream_parser.

Good point, I'm also a bit surprised. This code was added a while ago by
damienv@ and reviewed by acolwell@
(https://chromiumcodereview.appspot.com/23566013) both are gone from Chromium.
In this CL I've just tried to convert the build flags by mechanically replacing
strings. I'll look into guarding formats/mp2t by the new build flag/define.

[ddorwin, 2016-01-05 18:59:57]

Thanks. LG, but I have a question about the move of the GYP/GN variable. You
might want to have brettw take a look at that part too before submitting
depending on the answer.

https://codereview.chromium.org/1539983004/diff/60001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1539983004/diff/60001/media/BUILD.gn#newcode17
media/BUILD.gn:17: # Enables the MPEG2-TS stream parser for use with Media
Source. Disabled by
On 2016/01/05 00:18:09, servolk wrote:
> On 2016/01/04 22:19:45, ddorwin wrote:
> > This is consistent with the previous name but now more explicit. Are there
no
> > plans to support .src=HEVC?
> 
> Did you mean .src=foo.ts (mpeg2ts)? AFAIK there's no short-term plans to
support
> that, since almost all Chromecast apps use MPL player that works through MSE.

Yes, oops. SG.

https://codereview.chromium.org/1539983004/diff/140001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1539983004/diff/140001/media/media.gyp#newcode36
media/media.gyp:36: ['chromecast == 1', {
Ditto with the .GNI below.

https://codereview.chromium.org/1539983004/diff/140001/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/1539983004/diff/140001/media/media_options.gn...
media/media_options.gni:51: enable_mse_mpeg2ts_stream_parser = is_chromecast
I see you moved this and it is no longer inside buildflag_header with the
define. Is there a reason for this? Does it matter for the new mechanism?

[brettw, 2016-01-05 19:03:51]

https://codereview.chromium.org/1539983004/diff/140001/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/1539983004/diff/140001/media/media_options.gn...
media/media_options.gni:51: enable_mse_mpeg2ts_stream_parser = is_chromecast
On 2016/01/05 18:59:56, ddorwin wrote:
> I see you moved this and it is no longer inside buildflag_header with the
> define. Is there a reason for this? Does it matter for the new mechanism?

Like C, it doesn't matter where the variable is defined as long as it's defined
by the time you use it.

[ddorwin, 2016-01-05 19:17:24]

lgtm

https://codereview.chromium.org/1539983004/diff/140001/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/1539983004/diff/140001/media/media_options.gn...
media/media_options.gni:51: enable_mse_mpeg2ts_stream_parser = is_chromecast
On 2016/01/05 19:03:51, brettw wrote:
> On 2016/01/05 18:59:56, ddorwin wrote:
> > I see you moved this and it is no longer inside buildflag_header with the
> > define. Is there a reason for this? Does it matter for the new mechanism?
> 
> Like C, it doesn't matter where the variable is defined as long as it's
defined
> by the time you use it.

Thanks. I guess it was more a question of style and proximity.

[servolk, 2016-01-05 19:42:13]

On 2016/01/05 19:17:24, ddorwin wrote:
> lgtm
> 
> https://codereview.chromium.org/1539983004/diff/140001/media/media_options.gni
> File media/media_options.gni (right):
> 
>
https://codereview.chromium.org/1539983004/diff/140001/media/media_options.gn...
> media/media_options.gni:51: enable_mse_mpeg2ts_stream_parser = is_chromecast
> On 2016/01/05 19:03:51, brettw wrote:
> > On 2016/01/05 18:59:56, ddorwin wrote:
> > > I see you moved this and it is no longer inside buildflag_header with the
> > > define. Is there a reason for this? Does it matter for the new mechanism?
> > 
> > Like C, it doesn't matter where the variable is defined as long as it's
> defined
> > by the time you use it.
> 
> Thanks. I guess it was more a question of style and proximity.

It's also a question of scope/visibility. If you look at trybot results for PS
#6, you'll see a lot of GYP/GN failures, because enable_mse_mpeg2ts was defined
inside the buildflag_header scope and wasn't visible in the larger scope (so
when I tried to use it for another targer, e.g. the main 'media' target) I got
errors. Moving these variables back to media_options.gni and variables section
in media.gyp makes them visible/usable by other targets, besides
buildflag_header targets.

[servolk, 2016-01-05 19:43:40]

servolk@chromium.org changed reviewers:
+ jam@chromium.org
- jochen@chromium.org

[servolk, 2016-01-05 19:44:13]

On 2016/01/05 19:42:13, servolk wrote:
> On 2016/01/05 19:17:24, ddorwin wrote:
> > lgtm
> > 
> >
https://codereview.chromium.org/1539983004/diff/140001/media/media_options.gni
> > File media/media_options.gni (right):
> > 
> >
>
https://codereview.chromium.org/1539983004/diff/140001/media/media_options.gn...
> > media/media_options.gni:51: enable_mse_mpeg2ts_stream_parser = is_chromecast
> > On 2016/01/05 19:03:51, brettw wrote:
> > > On 2016/01/05 18:59:56, ddorwin wrote:
> > > > I see you moved this and it is no longer inside buildflag_header with
the
> > > > define. Is there a reason for this? Does it matter for the new
mechanism?
> > > 
> > > Like C, it doesn't matter where the variable is defined as long as it's
> > defined
> > > by the time you use it.
> > 
> > Thanks. I guess it was more a question of style and proximity.
> 
> It's also a question of scope/visibility. If you look at trybot results for PS
> #6, you'll see a lot of GYP/GN failures, because enable_mse_mpeg2ts was
defined
> inside the buildflag_header scope and wasn't visible in the larger scope (so
> when I tried to use it for another targer, e.g. the main 'media' target) I got
> errors. Moving these variables back to media_options.gni and variables section
> in media.gyp makes them visible/usable by other targets, besides
> buildflag_header targets.

-jochen@ (OOO), +jam@ for content/ approval.

[jam, 2016-01-07 00:53:02]

On 2016/01/05 19:44:13, servolk wrote:
> On 2016/01/05 19:42:13, servolk wrote:
> > On 2016/01/05 19:17:24, ddorwin wrote:
> > > lgtm
> > > 
> > >
> https://codereview.chromium.org/1539983004/diff/140001/media/media_options.gni
> > > File media/media_options.gni (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1539983004/diff/140001/media/media_options.gn...
> > > media/media_options.gni:51: enable_mse_mpeg2ts_stream_parser =
is_chromecast
> > > On 2016/01/05 19:03:51, brettw wrote:
> > > > On 2016/01/05 18:59:56, ddorwin wrote:
> > > > > I see you moved this and it is no longer inside buildflag_header with
> the
> > > > > define. Is there a reason for this? Does it matter for the new
> mechanism?
> > > > 
> > > > Like C, it doesn't matter where the variable is defined as long as it's
> > > defined
> > > > by the time you use it.
> > > 
> > > Thanks. I guess it was more a question of style and proximity.
> > 
> > It's also a question of scope/visibility. If you look at trybot results for
PS
> > #6, you'll see a lot of GYP/GN failures, because enable_mse_mpeg2ts was
> defined
> > inside the buildflag_header scope and wasn't visible in the larger scope (so
> > when I tried to use it for another targer, e.g. the main 'media' target) I
got
> > errors. Moving these variables back to media_options.gni and variables
section
> > in media.gyp makes them visible/usable by other targets, besides
> > buildflag_header targets.
> 
> -jochen@ (OOO), +jam@ for content/ approval.

since brettw is toplevel owner, his review counts here too

[servolk, 2016-01-07 00:53:49]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-01-07 00:53:50]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1539983004/#ps140001
(title: "Buildfix")

[commit-bot: I haz the power, 2016-01-07 00:54:25]

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

[commit-bot: I haz the power, 2016-01-07 00:56:10]

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

[commit-bot: I haz the power, 2016-01-07 00:56:11]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  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_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[servolk, 2016-01-07 01:59:50]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-01-07 01:59:50]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1539983004/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-07 02:00:47]

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

[commit-bot: I haz the power, 2016-01-07 03:20:03]

Description was changed from

==========
Convert enable_mpeg2ts_parser GYP flag into a new-style buildflag

According to recommendation given at
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5c4ySpPsV14

BUG=none
==========

to

==========
Convert enable_mpeg2ts_parser GYP flag into a new-style buildflag

According to recommendation given at
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5c4ySpPsV14

BUG=none
==========

[commit-bot: I haz the power, 2016-01-07 03:20:04]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-01-07 03:21:35]

Description was changed from

==========
Convert enable_mpeg2ts_parser GYP flag into a new-style buildflag

According to recommendation given at
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5c4ySpPsV14

BUG=none
==========

to

==========
Convert enable_mpeg2ts_parser GYP flag into a new-style buildflag

According to recommendation given at
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5c4ySpPsV14

BUG=none

Committed: https://crrev.com/2a41135331b37b94b15df5af641421cf407b2d11
Cr-Commit-Position: refs/heads/master@{#368001}
==========

[commit-bot: I haz the power, 2016-01-07 03:21:36]

Patchset 9 (id:??) landed as
https://crrev.com/2a41135331b37b94b15df5af641421cf407b2d11
Cr-Commit-Position: refs/heads/master@{#368001}