Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1569053002:
Raise gyp/GN failure if proprietary_codecs=1 and ffmpeg_branding=Chromium (Closed)
|
Created:
4 years, 11 months ago by Julien Isorce Samsung Modified:
4 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRaise gyp/GN failure if proprietary_codecs=1 and ffmpeg_branding=Chromium
Otherwise Chromium claims supporting video/mp4 and fails to play some
YouTube video without falling back to video/webm even if supported by
the server.
BUG=571417
R=jrummell@chromium.org
Committed: https://crrev.com/8f897728330ab4afe8c449001386756a9895bcf7
Cr-Commit-Position: refs/heads/master@{#412760}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address remarks #Messages
Total messages: 25 (8 generated)
jrummell@chromium.org changed reviewers: + dalecurtis@chromium.org
Comments added. +dalecurtis@ for his view on whether changes to build/common.gypi are desired. https://codereview.chromium.org/1569053002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1569053002/diff/1/build/common.gypi#newcode849 build/common.gypi:849: 'ffmpeg_branding%': "Chromium", I thought we were trying to keep ffmpeg_branding local to the media build files. However, I see there is already a reference to it in this file (line 1899), so maybe not. +dalecurtis@ for his opinion. https://codereview.chromium.org/1569053002/diff/1/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/1569053002/diff/1/media/BUILD.gn#newcode25 media/BUILD.gn:25: } Looks like assert() is the way to fail. Maybe remove ffmpeg_branding == "Chromium" from the if, and add: assert(ffmpeg_branding != "Chromium", "proprietary codecs and ffmpeg_branding set to Chromium are incompatible") https://codereview.chromium.org/1569053002/diff/1/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/1569053002/diff/1/media/media.gyp#newcode1137 media/media.gyp:1137: 'fail%': "<!(echo proprietary codecs and ffmpeg_branding set to Chromium are incompatible; exit 1)>", Not sure if fail needs to be a variable. 'fail' also seems to work, but I guess it doesn't make any difference.
I don't think we want to do this right now. I'm in the process of sorting out what Android should be built with as it pertains to webview. Currently we build with prop_codecs=1 always on Android even though ffmpeg_branding needs to be Chromium until discussed with legal. https://codereview.chromium.org/1569053002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1569053002/diff/1/build/common.gypi#newcode849 build/common.gypi:849: 'ffmpeg_branding%': "Chromium", On 2016/01/11 22:05:47, jrummell wrote: > I thought we were trying to keep ffmpeg_branding local to the media build files. > However, I see there is already a reference to it in this file (line 1899), so > maybe not. +dalecurtis@ for his opinion. That's a bug but may be something we end up needing to do here.
Thx for the reviews. On 2016/01/11 22:13:47, DaleCurtis wrote: > I don't think we want to do this right now. I'm in the process of sorting out > what Android should be built with as it pertains to webview. Any update ? > Currently we build > with prop_codecs=1 always on Android even though ffmpeg_branding needs to be > Chromium until discussed with legal. > Maybe I can make it skip the new failure if android ?
No, as with all things legal, it's a slow process :/
On 2016/01/23 00:05:52, DaleCurtis wrote: > No, as with all things legal, it's a slow process :/ Hi Dale, any update ? Thx.
Probably not happening any time soon, I'm okay with landing something like this, but this CL needs to be updated to be GN only now.
The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/17 20:45:56, DaleCurtis wrote: > Probably not happening any time soon, I'm okay with landing something like this, > but this CL needs to be updated to be GN only now. Done in Patch Set 2. Note that an alternative to this CL is to do a runtime check. Some months ago I prototyped it here https://codereview.chromium.org/1684853003/ . The important bit is in MimeUtil::IsCodecSupported from media/base/mime_util.cc . https://codereview.chromium.org/1569053002/diff/1/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/1569053002/diff/1/media/BUILD.gn#newcode25 media/BUILD.gn:25: } On 2016/01/11 22:05:47, jrummell wrote: > Looks like assert() is the way to fail. Maybe remove ffmpeg_branding == > "Chromium" from the if, and add: > > assert(ffmpeg_branding != "Chromium", "proprietary codecs and ffmpeg_branding > set to Chromium are incompatible") Done, thx.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by j.isorce@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Raise gyp/GN failure if proprietary_codecs=1 and ffmpeg_branding=Chromium Otherwise Chromium claims supporting video/mp4 and fails to play some YouTube video without falling back to video/webm even if supported by the server. BUG=571417 R=jrummell@chromium.org ========== to ========== Raise gyp/GN failure if proprietary_codecs=1 and ffmpeg_branding=Chromium Otherwise Chromium claims supporting video/mp4 and fails to play some YouTube video without falling back to video/webm even if supported by the server. BUG=571417 R=jrummell@chromium.org Committed: https://crrev.com/8f897728330ab4afe8c449001386756a9895bcf7 Cr-Commit-Position: refs/heads/master@{#412760} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8f897728330ab4afe8c449001386756a9895bcf7 Cr-Commit-Position: refs/heads/master@{#412760}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2260523002/ by j.isorce@samsung.com. The reason for reverting is: Failures on https://build.chromium.org/p/chromium.fyi/buildslaves/slave43-c1.
Message was sent while issue was closed.
Description was changed from ========== Raise gyp/GN failure if proprietary_codecs=1 and ffmpeg_branding=Chromium Otherwise Chromium claims supporting video/mp4 and fails to play some YouTube video without falling back to video/webm even if supported by the server. BUG=571417 R=jrummell@chromium.org Committed: https://crrev.com/8f897728330ab4afe8c449001386756a9895bcf7 Cr-Commit-Position: refs/heads/master@{#412760} ========== to ========== Raise gyp/GN failure if proprietary_codecs=1 and ffmpeg_branding=Chromium Otherwise Chromium claims supporting video/mp4 and fails to play some YouTube video without falling back to video/webm even if supported by the server. BUG=571417 R=jrummell@chromium.org Committed: https://crrev.com/8f897728330ab4afe8c449001386756a9895bcf7 Cr-Commit-Position: refs/heads/master@{#412760} ==========
On 2016/08/18 14:30:13, Julien Isorce wrote: > The reason for reverting is: Failures on > https://build.chromium.org/p/chromium.fyi/buildslaves/slave43-c1. Since some bots need use proprietary_codecs=1 + ffmpeg_branding="Chromium" this CL does not make sense so I am closing it. Note that for I think the right solution is to do a runtime check. Some months ago I prototyped it here https://codereview.chromium.org/1684853003/ . The important bit is in MimeUtil::IsCodecSupported from media/base/mime_util.cc . If interested just let me know.
Message was sent while issue was closed.
I don't think any bots need that configuration... I think you might have caught a real issue with them, but will take a closer look tomorrow.
Message was sent while issue was closed.
On 2016/08/19 01:08:46, DaleCurtis wrote: > I don't think any bots need that configuration... I think you might have caught > a real issue with them, but will take a closer look tomorrow. Agreed. See https://bugs.chromium.org/p/chromium/issues/detail?id=571417#c13.
On 2016/08/19 02:12:28, ddorwin wrote: > On 2016/08/19 01:08:46, DaleCurtis wrote: > > I don't think any bots need that configuration... I think you might have > caught > > a real issue with them, but will take a closer look tomorrow. > > Agreed. See https://bugs.chromium.org/p/chromium/issues/detail?id=571417#c13. Oh ok, in that I am re-opening the CL (no need to create a re-land one I think because there is no additional change), let me know when I can commit it. Thx.
On 2016/08/19 10:34:49, Julien Isorce wrote: > On 2016/08/19 02:12:28, ddorwin wrote: > > On 2016/08/19 01:08:46, DaleCurtis wrote: > > > I don't think any bots need that configuration... I think you might have > > caught > > > a real issue with them, but will take a closer look tomorrow. > > > > Agreed. See https://bugs.chromium.org/p/chromium/issues/detail?id=571417#c13. > > Oh ok, in that I am re-opening the CL (no need to create a re-land one I think > because there is no additional change), let me know when I can commit it. Thx. Landed here https://codereview.chromium.org/2260573004/ . Also see https://codereview.chromium.org/2260893002/ . |
