Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2848213003:
Fix building FFMPEG into Android (Closed)
DescriptionFix building FFMPEG into Android
A recent change (crrev.com/2808093008) did two things:
a) Change a cc file but not change the header accordingly
b) Add a new dependency but not update the gn accordingly
This change fixes those two issues to allow building
Chrome on Android with ffmpeg built in.
Review-Url: https://codereview.chromium.org/2848213003
Cr-Commit-Position: refs/heads/master@{#469417}
Committed: https://chromium.googlesource.com/chromium/src/+/4252105d0539d5e9968f42f21d3387dc834b09a4
Patch Set 1 #
Total comments: 2
Patch Set 2 : Make decrypting decoder non-Android again #
Messages
Total messages: 32 (9 generated)
kraush@amazon.com changed reviewers: + dalecurtis@chromium.org
Hi Dale, Can you take a look at this change to fix the Android build with ffmpeg built in? Thanks! Holger
dalecurtis@chromium.org changed reviewers: + xhwang@chromium.org
Hmm, are y'all using the decrypting versions of these decoders? +xhwang for commentary on what he wants to do here.
IIRC it's just that you're using ffmpeg video decoders; not that you have a custom CDM. So it seems like we just want these to be excluded on Android always w/o regard for disable video decoders flag.
On 2017/05/01 16:59:36, DaleCurtis wrote: > Hmm, are y'all using the decrypting versions of these decoders? +xhwang for > commentary on what he wants to do here. I think so. I'm pretty sure the .h change has to be that way - the ifdef guard in the .cc file has recently changed to reflect this (crrev linked in commit message) -> This is purely to get .cc and .h consistent again. As for the .gn part - I realize this is a change in functionality, but the build is otherwise broken with the changed dependencies in 2808093008. In terms of functionality, I've verified that it seems to work (videos that didn't work without built-in ffmpeg still work with it built in after this change; video and audio) What are your concerns? Are the decrypting decoders slower or in another way less preferable?
https://codereview.chromium.org/2848213003/diff/1/media/filters/decoder_selec... File media/filters/decoder_selector.h (right): https://codereview.chromium.org/2848213003/diff/1/media/filters/decoder_selec... media/filters/decoder_selector.h:82: #if !defined(DISABLE_FFMPEG_VIDEO_DECODERS) I saw this when working on the DecoderSelector and got confused. The previous defined(OS_ANDROID) check isn't super accurate. It's more like "if not supporting decrypting decoder in the CDM". This is also checked at run-time, but excluding it at build time will make things slightly simper/faster (I hope). So I have a question on https://codereview.chromium.org/2808093008, why do you want to touch Decrypting*Decoder, when you are trying disable FFmpeg*Decoder? They are two different decoders... Could you please
https://codereview.chromium.org/2848213003/diff/1/media/filters/decoder_selec... File media/filters/decoder_selector.h (right): https://codereview.chromium.org/2848213003/diff/1/media/filters/decoder_selec... media/filters/decoder_selector.h:82: #if !defined(DISABLE_FFMPEG_VIDEO_DECODERS) On 2017/05/01 17:10:00, xhwang_slow wrote: > I saw this when working on the DecoderSelector and got confused. The previous > defined(OS_ANDROID) check isn't super accurate. It's more like "if not > supporting decrypting decoder in the CDM". This is also checked at run-time, but > excluding it at build time will make things slightly simper/faster (I hope). > > So I have a question on https://codereview.chromium.org/2808093008, why do you > want to touch Decrypting*Decoder, when you are trying disable FFmpeg*Decoder? > They are two different decoders... > > Could you please Sorry, just to clarify: Are you saying the changes made to media/filters/decoder_selector.cc in https://codereview.chromium.org/2808093008 seem incorrect, because ffmpeg flags should not guard decrypting_decoder code? If so, should we loop in rduszynski@opera.com who made those changes? I'm unfortunately not sure what build configuration they use and thus why those changes were made :(
On 2017/05/01 18:14:59, kraush wrote: > https://codereview.chromium.org/2848213003/diff/1/media/filters/decoder_selec... > File media/filters/decoder_selector.h (right): > > https://codereview.chromium.org/2848213003/diff/1/media/filters/decoder_selec... > media/filters/decoder_selector.h:82: #if !defined(DISABLE_FFMPEG_VIDEO_DECODERS) > On 2017/05/01 17:10:00, xhwang_slow wrote: > > I saw this when working on the DecoderSelector and got confused. The previous > > defined(OS_ANDROID) check isn't super accurate. It's more like "if not > > supporting decrypting decoder in the CDM". This is also checked at run-time, > but > > excluding it at build time will make things slightly simper/faster (I hope). > > > > So I have a question on https://codereview.chromium.org/2808093008, why do you > > want to touch Decrypting*Decoder, when you are trying disable FFmpeg*Decoder? > > They are two different decoders... > > > > Could you please > > Sorry, just to clarify: Are you saying the changes made to > media/filters/decoder_selector.cc in https://codereview.chromium.org/2808093008 > seem incorrect, because ffmpeg flags should not guard decrypting_decoder code? Yes, this is what I meant. The change looks confusing to me. > If so, should we loop in mailto:rduszynski@opera.com who made those changes? > I'm unfortunately not sure what build configuration they use and thus why those > changes were made :( Sure, let's discuss with rduszynski@opera.com.
kraush@amazon.com changed reviewers: + rduszynski@opera.com
Hi rduszynski, Can you please take a look at the discussion and comment on the reasoning for your changes in regards to making a non-ffmpeg decoder depend on ffmpeg flags? Thanks! Holger
On 2017/05/01 18:52:39, kraush wrote: > Hi rduszynski, > > Can you please take a look at the discussion and comment on the reasoning for > your changes in regards to making a non-ffmpeg decoder depend on ffmpeg flags? > I'm on vacation till 03.05.2017, so I do not have access to the code, but as far as I remember there were some dependencies on the stuff disabled when disable_ffmpeg_decoders was set. We are using a config where decoders are disabled on linux build. That was what my change was about. There also may be that I misunderstood what the case was with decrypting decoder. I'll run some tests on 04.05 to confirm that. Either way, if you are able to compile "linux no decoders" with this change, then it probably was my mistake. > Thanks! > Holger Regards, Rafal
On 2017/05/02 11:52:30, rduszynski wrote: > On 2017/05/01 18:52:39, kraush wrote: > > Hi rduszynski, > > > > Can you please take a look at the discussion and comment on the reasoning for > > your changes in regards to making a non-ffmpeg decoder depend on ffmpeg flags? > > > > I'm on vacation till 03.05.2017, so I do not have access to the code, but as far > as I remember there were some dependencies on the stuff disabled when > disable_ffmpeg_decoders was set. > We are using a config where decoders are disabled on linux build. That was what > my change was about. > There also may be that I misunderstood what the case was with decrypting > decoder. > I'll run some tests on 04.05 to confirm that. > Either way, if you are able to compile "linux no decoders" with this change, > then it probably was my mistake. > > > Thanks! > > Holger > > Regards, > Rafal Thanks for the explanation Rafal! I'll try a build with the following gn flags, which I think is roughly what you are building: is_debug=true symbol_level = 1 fieldtrial_testing_like_official_build=true disable_ffmpeg_video_decoders=true Will let you know how it goes.
Hey everyone, Can you take a look at this new revision? I made DecryptingDecoder non-Android again as suggested by xhwang@. In general, this unfortunately looks mostly like a revert of Rafal's change with some minor changes to get it to compile, so I hope this is semantically correct. I did run a build of: - Android chrome_public_apk and media_unittests_apk with and without disable_ffmpeg_video_decoders - Linux chrome and media_unittests with and without disable_ffmpeg_video_decoders --> All of those compiled. (Which is also why my reply took so long - Linux builds are sloooow :) Let me know if you'd like me to try any other build targets or settings (I still have all the out directories lying around) Rafal: I would also appreciate if you could try this change with your exact gn flags and see if it works. Everyone: Let me know if you think I introduced some functional bugs with this change. Thanks everyone! - Holger
dalecurtis@chromium.org changed required reviewers: + xhwang@chromium.org
lgtm, xhwang?
(xhwang, we should probably add a new has_decrypt_only support flag).
On 2017/05/03 19:38:38, DaleCurtis_OOO_May_5_To_May23 wrote: > (xhwang, we should probably add a new has_decrypt_only support flag). Thanks for fixing this! The change LGTM. Dale: Agreed that we could add a BUILDFLAG for this. Or we can just always try to initialize DVD on Android, which will always fail. It's a small performance hit, but will make our code/test simpler.
On 2017/05/03 21:41:30, xhwang wrote: > On 2017/05/03 19:38:38, DaleCurtis_OOO_May_5_To_May23 wrote: > > (xhwang, we should probably add a new has_decrypt_only support flag). > > Thanks for fixing this! > > The change LGTM. > > Dale: Agreed that we could add a BUILDFLAG for this. Or we can just always try > to initialize DVD on Android, which will always fail. It's a small performance > hit, but will make our code/test simpler. Thanks everyone! :) I'll wait until Rafal is back tomorrow and has confirmed this revision works for him. In case I haven't heard anything by Friday, I'll assume it works and commit the change.
On 2017/05/03 at 21:41:30, xhwang wrote: > On 2017/05/03 19:38:38, DaleCurtis_OOO_May_5_To_May23 wrote: > > (xhwang, we should probably add a new has_decrypt_only support flag). > > Thanks for fixing this! > > The change LGTM. > > Dale: Agreed that we could add a BUILDFLAG for this. Or we can just always try to initialize DVD on Android, which will always fail. It's a small performance hit, but will make our code/test simpler. That also sgtm if it's effectively nil.
On 2017/05/03 14:30:34, kraush wrote: > Hey everyone, > > Can you take a look at this new revision? > I made DecryptingDecoder non-Android again as suggested by xhwang@. > In general, this unfortunately looks mostly like a revert of Rafal's change with > some minor changes to get it to compile, so I hope this is semantically correct. > I did run a build of: > - Android chrome_public_apk and media_unittests_apk with and without > disable_ffmpeg_video_decoders > - Linux chrome and media_unittests with and without > disable_ffmpeg_video_decoders > > --> All of those compiled. (Which is also why my reply took so long - Linux > builds are sloooow :) > > Let me know if you'd like me to try any other build targets or settings (I still > have all the out directories lying around) > > Rafal: I would also appreciate if you could try this change with your exact gn > flags and see if it works. > > Everyone: Let me know if you think I introduced some functional bugs with this > change. > > Thanks everyone! > - Holger Hey, I can confirm that everything seems to be OK on our side. Seems that removing Decrypting*Decoders was too much on my side, where removing only ffmpeg decoders would suffice
On 2017/05/04 16:36:17, rduszynski wrote: > > I can confirm that everything seems to be OK on our side. Seems that removing > Decrypting*Decoders was too much on my side, where removing only ffmpeg decoders > would suffice Great, thanks Rafal! :) Then I'll commit this revision.
The CQ bit was checked by kraush@amazon.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
On 2017/05/04 18:22:57, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) The clang build failed on a Java test - both the "with patch" and "without patch" build. :( --> Don't think the failure is related. I'll retrigger the job, since the latest Android clang builds seem to have passed.
The CQ bit was checked by kraush@amazon.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493922755744530,
"parent_rev": "fe9e5c8eb8b0f7673827718ed72050c0d0b65147", "commit_rev":
"4252105d0539d5e9968f42f21d3387dc834b09a4"}
Message was sent while issue was closed.
Description was changed from ========== Fix building FFMPEG into Android A recent change (crrev.com/2808093008) did two things: a) Change a cc file but not change the header accordingly b) Add a new dependency but not update the gn accordingly This change fixes those two issues to allow building Chrome on Android with ffmpeg built in. ========== to ========== Fix building FFMPEG into Android A recent change (crrev.com/2808093008) did two things: a) Change a cc file but not change the header accordingly b) Add a new dependency but not update the gn accordingly This change fixes those two issues to allow building Chrome on Android with ffmpeg built in. Review-Url: https://codereview.chromium.org/2848213003 Cr-Commit-Position: refs/heads/master@{#469417} Committed: https://chromium.googlesource.com/chromium/src/+/4252105d0539d5e9968f42f21d33... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/4252105d0539d5e9968f42f21d33... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
