The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/c94da9e8deeded23aa92299844f619d1cf5a2022

commit c94da9e8deeded23aa92299844f619d1cf5a2022
Author: Dirk Pranke <dpranke@chromium.org>
Date: Tue Dec 19 02:29:54 2017

Change the way the ffmpeg_branding gets its defaults.

It looks like the logic for setting ffmpeg_branding should really
be keyed off of whether we want the proprietary_codecs and whether
we're doing a ChromeOS build, rather than whether it's actually
chrome-branded or not. So, rather than looking at is_chrome_branded,
we look directly at proprietary_codecs.

This change also removes some logic for ChromeCast that was no longer
needed but never cleaned up.

R=chcunningham@chromium.org
BUG= 570754 ,  795935 

Change-Id: I5e6ccbca915f06b598e22eda49683fff27e28fae
Reviewed-on: https://chromium-review.googlesource.com/832851
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>

[modify] https://crrev.com/c94da9e8deeded23aa92299844f619d1cf5a2022/ffmpeg_options.gni

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e65278298429af56ed3022b807bd3208ec1ec9ea

commit e65278298429af56ed3022b807bd3208ec1ec9ea
Author: Dirk Pranke <dpranke@chromium.org>
Date: Wed Dec 20 17:47:48 2017

Revert "Enable proprietary_codecs=true by default."

This reverts commit 7f446eb0ecff4014853501cd6d50fcf8ff6fa98b.

Reason for revert: reverting for now, now that we've established that it seems to work.

Original change's description:
> Enable proprietary_codecs=true by default.
>
> We want most of our test coverage to be done against builds with
> the proprietary codecs included, so this changes the default to
> do so. However, it is important that we *not* publicize or distribute
> any builds that includes these codecs (since they likely won't have
> the correct licensing), and so we explicitly turn the flag off on
> the builders that do publish builds.
>
> This change includes a roll of the FFmpeg repo from 423f74fa..c94da9e8
> in order to pick up that change, which changes ffmpeg to pick up
> `ffmpeg_branding`'s default value directly from `proprietary_codecs`,
> making the actual ffmpeg_branding GN arg kinda pointless (the branding
> is now driven by proprietary_codecs and is_chromeos).
>
> R=​dalecurtis@chromium.org, chcunningham@chromium.org, jbudorick@chromium.org, kbr@chromium.org
>
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> Change-Id: Id0b6cccbd235875c9386a527cb39c8f17d2dab03
> Reviewed-on: https://chromium-review.googlesource.com/832383
> Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
> Reviewed-by: Kenneth Russell <kbr@chromium.org>
> Commit-Queue: Dirk Pranke <dpranke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#525172}

TBR=dalecurtis@chromium.org,dpranke@chromium.org,kbr@chromium.org,chcunningham@chromium.org,jbudorick@chromium.org
BUG= 795935 

Change-Id: I30b9d447facc94f3eaaa55c7252e3f050720619f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/836414
Commit-Queue: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525364}
[modify] https://crrev.com/e65278298429af56ed3022b807bd3208ec1ec9ea/DEPS
[modify] https://crrev.com/e65278298429af56ed3022b807bd3208ec1ec9ea/build/config/features.gni
[modify] https://crrev.com/e65278298429af56ed3022b807bd3208ec1ec9ea/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[modify] https://crrev.com/e65278298429af56ed3022b807bd3208ec1ec9ea/tools/mb/mb_config.pyl

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2eb71b67e66737aa1fd7a0b71fe2daedd2bd7bea

commit 2eb71b67e66737aa1fd7a0b71fe2daedd2bd7bea
Author: Roger McFarlane <rogerm@chromium.org>
Date: Wed Dec 20 19:14:48 2017

Revert "Revert "Enable proprietary_codecs=true by default.""

This reverts commit e65278298429af56ed3022b807bd3208ec1ec9ea.

Reason for revert: looks like it's causing consistent layout test failures on Linux Trusty

Original change's description:
> Revert "Enable proprietary_codecs=true by default."
> 
> This reverts commit 7f446eb0ecff4014853501cd6d50fcf8ff6fa98b.
> 
> Reason for revert: reverting for now, now that we've established that it seems to work.
> 
> Original change's description:
> > Enable proprietary_codecs=true by default.
> >
> > We want most of our test coverage to be done against builds with
> > the proprietary codecs included, so this changes the default to
> > do so. However, it is important that we *not* publicize or distribute
> > any builds that includes these codecs (since they likely won't have
> > the correct licensing), and so we explicitly turn the flag off on
> > the builders that do publish builds.
> >
> > This change includes a roll of the FFmpeg repo from 423f74fa..c94da9e8
> > in order to pick up that change, which changes ffmpeg to pick up
> > `ffmpeg_branding`'s default value directly from `proprietary_codecs`,
> > making the actual ffmpeg_branding GN arg kinda pointless (the branding
> > is now driven by proprietary_codecs and is_chromeos).
> >
> > R=​dalecurtis@chromium.org, chcunningham@chromium.org, jbudorick@chromium.org, kbr@chromium.org
> >
> > Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> > Change-Id: Id0b6cccbd235875c9386a527cb39c8f17d2dab03
> > Reviewed-on: https://chromium-review.googlesource.com/832383
> > Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
> > Reviewed-by: Kenneth Russell <kbr@chromium.org>
> > Commit-Queue: Dirk Pranke <dpranke@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#525172}
> 
> TBR=dalecurtis@chromium.org,dpranke@chromium.org,kbr@chromium.org,chcunningham@chromium.org,jbudorick@chromium.org
> BUG= 795935 
> 
> Change-Id: I30b9d447facc94f3eaaa55c7252e3f050720619f
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> Reviewed-on: https://chromium-review.googlesource.com/836414
> Commit-Queue: Dirk Pranke <dpranke@chromium.org>
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#525364}

TBR=dalecurtis@chromium.org,dpranke@chromium.org,kbr@chromium.org,chcunningham@chromium.org,jbudorick@chromium.org

Change-Id: I4d8101bdab9fe17ad96d3e1fdaaa62626181769e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  795935 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/836670
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Commit-Queue: Roger McFarlane <rogerm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525398}
[modify] https://crrev.com/2eb71b67e66737aa1fd7a0b71fe2daedd2bd7bea/DEPS
[modify] https://crrev.com/2eb71b67e66737aa1fd7a0b71fe2daedd2bd7bea/build/config/features.gni
[modify] https://crrev.com/2eb71b67e66737aa1fd7a0b71fe2daedd2bd7bea/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[modify] https://crrev.com/2eb71b67e66737aa1fd7a0b71fe2daedd2bd7bea/tools/mb/mb_config.pyl

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/696298979b501b7bb06776e0e95f053f8d53a86d

commit 696298979b501b7bb06776e0e95f053f8d53a86d
Author: Dirk Pranke <dpranke@chromium.org>
Date: Wed Dec 20 20:16:56 2017

Re-Revert "Enable proprietary_codecs=true by default."

This reverts commit 2eb71b67e66737aa1fd7a0b71fe2daedd2bd7bea.

When we reverted the proprietary_codecs=true change the first time,
that caused some layout tests to fail because we had landed 
crrev.com/827659 (which depended on the flag being true).

We have now reverted 827659, in crrev.com/c/837247, so it should
be safe to re-revert this here.

Original change's description:
> Revert "Revert "Enable proprietary_codecs=true by default.""
> 
> This reverts commit e65278298429af56ed3022b807bd3208ec1ec9ea.
> 
> Reason for revert: looks like it's causing consistent layout test failures on Linux Trusty
> 
> Original change's description:
> > Revert "Enable proprietary_codecs=true by default."
> > 
> > This reverts commit 7f446eb0ecff4014853501cd6d50fcf8ff6fa98b.
> > 
> > Reason for revert: reverting for now, now that we've established that it seems to work.
> > 
> > Original change's description:
> > > Enable proprietary_codecs=true by default.
> > >
> > > We want most of our test coverage to be done against builds with
> > > the proprietary codecs included, so this changes the default to
> > > do so. However, it is important that we *not* publicize or distribute
> > > any builds that includes these codecs (since they likely won't have
> > > the correct licensing), and so we explicitly turn the flag off on
> > > the builders that do publish builds.
> > >
> > > This change includes a roll of the FFmpeg repo from 423f74fa..c94da9e8
> > > in order to pick up that change, which changes ffmpeg to pick up
> > > `ffmpeg_branding`'s default value directly from `proprietary_codecs`,
> > > making the actual ffmpeg_branding GN arg kinda pointless (the branding
> > > is now driven by proprietary_codecs and is_chromeos).
> > >
> > > R=​dalecurtis@chromium.org, chcunningham@chromium.org, jbudorick@chromium.org, kbr@chromium.org
> > >
> > > Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> > > Change-Id: Id0b6cccbd235875c9386a527cb39c8f17d2dab03
> > > Reviewed-on: https://chromium-review.googlesource.com/832383
> > > Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
> > > Reviewed-by: Kenneth Russell <kbr@chromium.org>
> > > Commit-Queue: Dirk Pranke <dpranke@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#525172}
> > 
> > TBR=dalecurtis@chromium.org,dpranke@chromium.org,kbr@chromium.org,chcunningham@chromium.org,jbudorick@chromium.org
> > BUG= 795935 
> > 
> > Change-Id: I30b9d447facc94f3eaaa55c7252e3f050720619f
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> > Reviewed-on: https://chromium-review.googlesource.com/836414
> > Commit-Queue: Dirk Pranke <dpranke@chromium.org>
> > Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#525364}
> 
> TBR=dalecurtis@chromium.org,dpranke@chromium.org,kbr@chromium.org,chcunningham@chromium.org,jbudorick@chromium.org
> 
> Change-Id: I4d8101bdab9fe17ad96d3e1fdaaa62626181769e
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  795935 
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> Reviewed-on: https://chromium-review.googlesource.com/836670
> Reviewed-by: Roger McFarlane <rogerm@chromium.org>
> Commit-Queue: Roger McFarlane <rogerm@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#525398}

TBR=dalecurtis@chromium.org,rogerm@chromium.org,dpranke@chromium.org,kbr@chromium.org,chcunningham@chromium.org,jbudorick@chromium.org

Change-Id: I36a30feaa5fe52633fecde9ebeea29db7dbc3563
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  795935 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/837513
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525426}
[modify] https://crrev.com/696298979b501b7bb06776e0e95f053f8d53a86d/DEPS
[modify] https://crrev.com/696298979b501b7bb06776e0e95f053f8d53a86d/build/config/features.gni
[modify] https://crrev.com/696298979b501b7bb06776e0e95f053f8d53a86d/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[modify] https://crrev.com/696298979b501b7bb06776e0e95f053f8d53a86d/tools/mb/mb_config.pyl

Don't have time to review this more sure to vacation, but if this does what I think it does (default build has non free codecs) I think is the wrong change and should be reverted immediately. It's fail-bad for any embedder or automatic builder of chromium in ways that need clarification from legal.

@dalecurtis - the change was reverted (i.e., proprietary_codecs is off by default now) and I agree that we want to talk this over w/ legal (I cc'ed you on the note to them); however, I thought we had agreed to turn it on by default?

Thanks for confirming. I thought that agreement was to turn on for bots only, but this seems to turn on for checkouts. Which I don't think should be done for many reasons, happy to chat more in January. Bots are fine though.

Yup, assuming there's no urgency to Chris's fix, this can wait.

There's a bunch of discussion on the tradeoffs between kbr@, chcunningham@, and myself in https://crrev.com/c/832383.

Quoting from one of my comments there: 

If we were to set this to be true on the bots but not by default in a dev checkout, then we'd be likely to see failures on the bots that devs wouldn't see (by default) locally. I think it was okay for *some* bots to have this, but if we're changing things such that *nearly every* bot has this, it should be the default in the build.

Put differently, I think that if we're not okay with this, we shouldn't set this on on all of the bots by default, and if we're not setting it on on all of the bots by default, I'm not okay with the layout tests depending on it (because I don't want to have to change the configuration of the bot based on which test it is running [particularly for such a central test suite as the layout tests]).

Downgrading to P2 for the moment, until we can discuss more fully.

If the main concern is mainly the legal one of downstream projects' Chromium-based builds, then I would vote for leaving the build's default to proprietary_codeces=false, and changing all the Chromium bots' configurations to build with proprietary_codecs=true *except* for those on the "chromium" waterfall. It should be hard to add a new Chromium bot with proprietary_codecs=false.

Enough announcements about this flag should be sufficient to get all the Chromium developers to set proprietary_codecs=true in their own local builds, in my opinion. Perhaps it's technically feasible to also set it somehow during "fetch chromium"?

I'm not a fan of asking regular chromium devs to have to set flags in order to get "the default build". 

I don't know what the legal implications of changing the default are; I've started a thread w/ people to find out. I don't know if setting it automatically during `fetch chromium` would be problematic or not.

It's good that the legal question has been raised.
I would personally expect the default-build of a chromium checkout to not include proprietary stuff, and I would think that changing the default build behavior would cause many downstream projects to inadvertently start including these things without these projects realizing it or understanding the implications.

+1 to kbr for c#11.

Regardless of what our legal team says, the fact that we need a discussion at all with legal means it's problematic to flip the default value to true. As I indicated above, this is a fail-bad situation for other projects.

The default is only fail bad for a project that doesn't want to do any diligence about what it's building before redistributing it. I'm not sure that such a project exists, and it probably shouldn't. I don't think accommodating that project should be our default case, and this is something that can be trivially handled by downstream projects.

> the fact that we need a discussion at all with legal means it's problematic
> to flip the default value to true. 

Having to think about the legal implications of doing something doesn't mean that it's not the right thing to do :).

It's not standard practice for open source projects to build non-free things by default (see ffmpeg, vlc, etc), so this is an unexpected consideration for a downstream project. As such it seems an unnecessary foot-gun to make this change; especially since we've followed standard practice since the dawn of the project.

That's understandable.

To re-iterate:

1. We got here because we wanted to modify the layout tests so that tests for the proprietary codecs would pass.

2. I am not okay with us modifying the layout tests to fail by default for a dev checkout.

3. I would strongly prefer that what we get with a default dev checkout is what we run on the bots.

4. I strongly prefer that we not test and build things that aren't what we ship, if we don't have to. This means that I don't like that we test both with and without proprietary codecs, and I would prefer to simplify this by removing both configurations.

I do not know how to make all of these things true if proprietary_codecs is off by default, unless we modify `fetch chromium` to add it, and we then have to modify all the build configs (which don't use fetch) to add it. Doing that work is possibly acceptable, but feels goofy and confusing and worse to me than just changing the default GN arg.

I am not suggesting that WebRTC, ffmpeg, vlc, etc. change how they do things, but I think there are relatively few people that check out Chromium that don't want the proprietary codecs (compared to either the people that don't care, or the people that actually do want the codecs), and I don't think catering to them makes sense over the above goals.

Some context is missing on #2, though you mention it in #1. The tests already fail, which is unfortunate I agree. The real fix here is that all tests should be forced to use non-proprietary codecs and clear canplaytype strings. I definitely agree that we should not check in something that breaks layout tests for the default build. I didn't think that's what we're doing here though. chcunningham@ can you confirm?

I also think 3/4 are well-intentioned (and I do agree with them predominantly) but overlook salient details of our existing world vis-a-vis official and chrome branded builds vs chromium. I.e. these values should be synonymous with an official build, but are not for developer convenience and cq coverage only. Chromium the non-official build will never include all Chrome things, though we try to minimize this gap. Obviously my position is that proprietary codecs are not one of the things that can be put into Chromium.

(Further notes: We have the prop_codecs+ffmpeg_branding values since building a Chrome branded build requires a bunch of additional stuff; some of which was hard to acquire in the past. Official builds also do not have as much test coverage either, so we've flipped it for some bots in the past to ensure breakages don't make it past the CQ)

> The real fix here is that all tests should be forced to use non-proprietary 
> codecs and clear canplaytype strings. 

This would be equally fine (for purposes of addressing 1., and is effectively 
where are today, because we just have Fail in the TestExpectations for them).

> I definitely agree that we should not check in something that breaks layout
> tests for the default build. I didn't think that's what we're doing here
> though. chcunningham@ can you confirm?

chcunningham's change would've broken the default build (it requires GN flags to be set).

Just to be clear (and pedantic): "we can't set the flag to true" and "I don't want the
flag to be set to true" are two different things. You're clearly saying the latter (
which is fine, even though we're disagreeing); 

Are you *also* saying the former? As far as I know, the former isn't true (i.e., we
could set the flag to true if we wanted to).

>chcunningham's change would've broken the default build (it requires GN flags to be set).

Ah, I missed that his change would break the build, that's not okay then. These tests must pass with the standard configuration. I just chatted with Chris+Matt and we concluded he'll change the tests to use correct canPlayType strings; e.g., something like 'video/mp4, codecs="avc1"' vs just 'video/mp4' which is now ambiguous. For tests which are hard-coded to use h264+mp4 and currently always fail, we'll change the test expectations to pass||fail which provides nominal coverage for crashes and is an improvement over the current complete failure. 

> Just to be clear (and pedantic): "we can't set the flag to true" and "I don't want the
> flag to be set to true" are two different things. You're clearly saying the latter (
> which is fine, even though we're disagreeing);
>
> Are you *also* saying the former? As far as I know, the former isn't true (i.e., we
> could set the flag to true if we wanted to).

Sorry, I've tried to carefully phrase my statements to indicate it's my opinion that we shouldn't flip the flag to true by default given open-source project norms. I've intentionally avoided speculation on whether we can do this beyond noting that it's fail-bad.

> I've intentionally avoided speculation on whether we can do this beyond 
> noting that it's fail-bad.

Ack, thanks. That was what I thought, just checking. I don't know whether we can do this or not, either, I just also didn't think that we didn't know for sure that we couldn't not do it :).

I'm going to mark this as closed/WontFix for now. I don't think we reached a definitive conclusion here, but the pressing need to change the configurations has been dealt with in other ways, so we don't need to change anything at the moment.

Thank you for taking implications for downstream projects into account :)

It seems c94da9e8deeded23aa92299844f619d1cf5a2022 is still not reverted from the M65 branch.  It would probably be a good idea to revert it there too.

> It seems c94da9e8deeded23aa92299844f619d1cf5a2022 is still not
> reverted from the M65 branch.

That change affected how variables were initialized, but didn't actually change the proprietary_codecs setting. It was never reverted and shouldn't need to be?

Right, it's still on master, too.

The thing is, proprietary_codecs traditionally used to enable codepaths involved in the playback of proprietary formats without actually adding the codecs themselves into the build, see https://www.chromium.org/audio-video.  This made sense, because there are ways to provide the necessary decoding capabilities using, e.g., OS libraries instead of FFmpeg.

If we're talking about legal considerations then the bundling of the codecs is what matters.  With this change, it's easy for a downstream project to inadvertently start bundling proprietary codecs.

I see. I didn't realize that was how the distinction was drawn.

I defer to dalecurtis@ to whether we need to revert that change or not.

Hmm, we emit an error if you try to set proprietary codecs w/o ffmpeg_branding when ffmpeg is used. So I don't think this changes anything.

https://cs.chromium.org/chromium/src/media/BUILD.gn?l=41

I cannot say I agree, unfortunately.

Before c94da9e8deeded23aa92299844f619d1cf5a2022 the distinction between proprietary_codecs and ffmpeg_branding used to be as described in https://www.chromium.org/audio-video. c94da9e8deeded23aa92299844f619d1cf5a2022 alters the rules laid out in https://www.chromium.org/audio-video (and re-iterated on chromium-dev numerous times) by giving proprietary_codecs the additional meaning of also enabling the proprietary codecs in FFmpeg by default.

I'm aware of https://cs.chromium.org/chromium/src/media/BUILD.gn?rcl=c9f631add3cbe4c285f81c298b0c571ceabfc148&l=42 and I'm also not a fan of it, because what it's saying is not true for any downstream project that provides proprietary codecs from outside of FFmpeg (and it needs to be patched out).

One could brush c94da9e8deeded23aa92299844f619d1cf5a2022 off as just another place to patch / another GN variable to set. However, what sets it apart is that now you have to know that you need to patch / set a variable even though the common knowledge has been that you don't have to. ffmpeg_branding used to be implicitly "Chromium" if you're not Chrome, now it's (sometimes) implicitly "Chrome". From my point of view it's a change with potentially far-reaching implications and at the same time one that is easy to go unnoticed until it's too late.

I see your concerns, but while that error is emitted there is no worry that someone will accidentally ship a proprietary codec enabled ffmpeg.

I also don't think we'll remove the error either since by far the most common complaint we got for years before adding that was: "why isn't (vimeo|youtube|facebook) playing in my custom XYZ build? I set proprietary_codecs???"

I'm open to any changes here that make this flow better, but right now we seem to be having a theoretical discussion in which someone has deleted that error. Did this issue occur in practice? I'm open to reverting the other patch, but don't want to revert something over speculation.

> Did this issue occur in practice?

I can tell you I'm not commenting here by accident :)

It was, however, pure luck that I learned about the switching of the default ffmpeg_branding in time. If I sound too adamant and whiny about the whole thing then I'm sorry. Still, I think it could have been communicated better.

The reason I find it easy to ship the wrong kind of FFmpeg build is because I think product testing is typically focused on the case where the environment provides the necessary bits and the expected result is that the browser handles proprietary codecs. One doesn't often get to test the negative case that makes sure they don't work when the environment doesn't provide them.

An important lesson for me from all this is that, yes, tests of the latter kind are important.

I also understand your position on the benefits of the new ffmpeg_branding default value. It seems deciding what the default should be is a matter of optimizing either for the user of a personal, custom build, or a downstream project that actually ships a product. But maybe the custom build user is provided by the GN error with enough information to make an informed decision about the FFmpeg branding they want to use themselves?

Sorry this fell off my plate. I don't think the patch actually changes anything (due to the error message) and it's proving contentious, so I'm fine with reverting it from ToT at least.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/9ba23a78bb44f35dbe7e49183c8e21699943f2de

commit 9ba23a78bb44f35dbe7e49183c8e21699943f2de
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Mon Feb 26 20:17:03 2018

Revert "Change the way the ffmpeg_branding gets its defaults."

This reverts commit c94da9e8deeded23aa92299844f619d1cf5a2022.

Reason for revert: Doesn't change anything due to an error message generated in another file and is contentious with downstream users, so revert.

Original change's description:
> Change the way the ffmpeg_branding gets its defaults.
> 
> It looks like the logic for setting ffmpeg_branding should really
> be keyed off of whether we want the proprietary_codecs and whether
> we're doing a ChromeOS build, rather than whether it's actually
> chrome-branded or not. So, rather than looking at is_chrome_branded,
> we look directly at proprietary_codecs.
> 
> This change also removes some logic for ChromeCast that was no longer
> needed but never cleaned up.
> 
> R=​chcunningham@chromium.org
> BUG= 570754 ,  795935 
> 
> Change-Id: I5e6ccbca915f06b598e22eda49683fff27e28fae
> Reviewed-on: https://chromium-review.googlesource.com/832851
> Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>

TBR=dpranke@chromium.org,chcunningham@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  570754 ,  795935 
Change-Id: I1ce31b755f66e41ac2a1cb4291e6f54bb356c0c3
Reviewed-on: https://chromium-review.googlesource.com/938201
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>

[modify] https://crrev.com/9ba23a78bb44f35dbe7e49183c8e21699943f2de/ffmpeg_options.gni

Marking this as WontFix, since I don't think we should do this and the lawyers never got back to us (or at least me). We can re-open if that changes.

> I don't think the patch actually changes anything (due to the error message)
> and it's proving contentious, so I'm fine with reverting it from ToT at least.

Great, thanks!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d063a3e1dec3f895e244ffad122dbe32399966d6

commit d063a3e1dec3f895e244ffad122dbe32399966d6
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Sat Mar 03 01:25:33 2018

Roll src/third_party/ffmpeg/ 9ca66eafa..ef99a5d25 (2 commits)

https://chromium.googlesource.com/chromium/third_party/ffmpeg.git/+log/9ca66eafaf6f..ef99a5d2520f

$ git log 9ca66eafa..ef99a5d25 --date=short --no-merges --format='%ad %ae %s'
2018-03-02 wolenetz ffmpeg: Initialize a potential gap in ctts_data in mov_build_index
2018-02-26 dalecurtis Revert "Change the way the ffmpeg_branding gets its defaults."

Created with:
  roll-dep src/third_party/ffmpeg
BUG= 816787 , 570754 , 795935 

Change-Id: I44bcdd293b786cdc4e754f81e82695578b0ba6c3
Reviewed-on: https://chromium-review.googlesource.com/947386
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540707}
[modify] https://crrev.com/d063a3e1dec3f895e244ffad122dbe32399966d6/DEPS