Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2156193003:
[Chromecast] Add a custom CastMediaClient for Chromecast (Closed)
|
Created:
4 years, 5 months ago by servolk Modified:
4 years, 4 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, gfhuang+watch_chromium.org, halliwell+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@codecs Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Add a custom CastMediaClient for Chromecast
It will forward MediaClient::IsSupportedVideoConfig invocations to a
shared library implementation, which will allow us to have different
codecs supported on different platforms.
BUG=477103
Committed: https://crrev.com/29ce77106c738a40237b4a1715f953913e493413
Cr-Commit-Position: refs/heads/master@{#408300}
Patch Set 1 #Patch Set 2 : Added enum value conversion + various buildfixes #Patch Set 3 : fixed deps #Patch Set 4 : Remove direct dependency on content::RenderMediaClient #Patch Set 5 : buildfix #Patch Set 6 : Added IsSupportedVideoConfig to cast_media_dummy.cc #Patch Set 7 : Fixed unit tests #Patch Set 8 : Move CastMediaClient::Init to RenderThreadStarted #Patch Set 9 : rebase #Patch Set 10 : rebase #Patch Set 11 : Move #define MEDIA_IMPLEMENTATION to .cc file #
Total comments: 2
Patch Set 12 : Mark MediaCapShlib::IsSupportedVideoConfig as weak symbol #
Total comments: 12
Patch Set 13 : CR fixes #
Total comments: 4
Patch Set 14 : Move MediaCapShlib to a separate header #Patch Set 15 : Added comments for MediaCapShlib interface #Patch Set 16 : rebase #
Total comments: 2
Messages
Total messages: 93 (68 generated)
The CQ bit was checked by servolk@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by servolk@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by servolk@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by servolk@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by servolk@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by servolk@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by servolk@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by servolk@chromium.org 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...
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 servolk@chromium.org 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...
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 servolk@chromium.org 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...
Description was changed from ========== [Chromecast] Add a custom CastMediaClient for Chromecast It will forward MediaClient::IsSupportedVideoConfig invocations to a shared library implementation, which will allow us to have different codecs supported on different platforms. BUG=477103 ========== to ========== [Chromecast] Add a custom CastMediaClient for Chromecast It will forward MediaClient::IsSupportedVideoConfig invocations to a shared library implementation, which will allow us to have different codecs supported on different platforms. BUG=477103 ==========
servolk@chromium.org changed reviewers: + erickung@chromium.org, halliwell@chromium.org
On 2016/07/26 21:43:21, servolk wrote: > mailto:servolk@chromium.org changed reviewers: > + mailto:erickung@chromium.org, mailto:halliwell@chromium.org PTAL, this should allow us to get rid of the current downstream hacks for codec checks and instead rely on this new custom MediaClient and shlib implementations for platform-specific codec checks.
gfhuang@chromium.org changed reviewers: + gfhuang@chromium.org
https://codereview.chromium.org/2156193003/diff/200001/chromecast/public/medi... File chromecast/public/media_codec_support_shlib.h (right): https://codereview.chromium.org/2156193003/diff/200001/chromecast/public/medi... chromecast/public/media_codec_support_shlib.h:43: this public API change is not safe for OEM Cast-only update.
https://codereview.chromium.org/2156193003/diff/200001/chromecast/public/medi... File chromecast/public/media_codec_support_shlib.h (right): https://codereview.chromium.org/2156193003/diff/200001/chromecast/public/medi... chromecast/public/media_codec_support_shlib.h:43: On 2016/07/26 21:48:58, gfhuang wrote: > this public API change is not safe for OEM Cast-only update. Yes, I guess we can mark it as '__attribute__((__weak__))' for now and only implement it on platforms where we really need it and then later (when we are updating vendor interfaces) make it mandatory on all platforms.
The CQ bit was checked by servolk@chromium.org 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/07/26 22:13:45, servolk wrote: > https://codereview.chromium.org/2156193003/diff/200001/chromecast/public/medi... > File chromecast/public/media_codec_support_shlib.h (right): > > https://codereview.chromium.org/2156193003/diff/200001/chromecast/public/medi... > chromecast/public/media_codec_support_shlib.h:43: > On 2016/07/26 21:48:58, gfhuang wrote: > > this public API change is not safe for OEM Cast-only update. > > Yes, I guess we can mark it as '__attribute__((__weak__))' for now and only > implement it on platforms where we really need it and then later (when we are > updating vendor interfaces) make it mandatory on all platforms. I've done this in patchset #8, PTAL
On 2016/07/26 22:42:15, servolk wrote: > On 2016/07/26 22:13:45, servolk wrote: > > > https://codereview.chromium.org/2156193003/diff/200001/chromecast/public/medi... > > File chromecast/public/media_codec_support_shlib.h (right): > > > > > https://codereview.chromium.org/2156193003/diff/200001/chromecast/public/medi... > > chromecast/public/media_codec_support_shlib.h:43: > > On 2016/07/26 21:48:58, gfhuang wrote: > > > this public API change is not safe for OEM Cast-only update. > > > > Yes, I guess we can mark it as '__attribute__((__weak__))' for now and only > > implement it on platforms where we really need it and then later (when we are > > updating vendor interfaces) make it mandatory on all platforms. > > I've done this in patchset #8, PTAL Oops, sorry, I meant patchset #12
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/medi... chromecast/public/media/decoder_config.h:87: kHEVCMainStillPicture, this should be added at the bottom, otherwise it will not be backward compatible. https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/medi... File chromecast/public/media_codec_support_shlib.h (right): https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/medi... chromecast/public/media_codec_support_shlib.h:10: #include "chromecast/public/media/decoder_config.h" remove "chromecast/public" and use relative path, otherwise it will cause issues when OEMs include this.
https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... File chromecast/common/media/cast_media_client.cc (right): https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... chromecast/common/media/cast_media_client.cc:9: #define MEDIA_IMPLEMENTATION does this break component build? Also, it's a pre-existing problem, but it definitely seems quite ugly how RenderMediaClient gets instantiated :( Could we subclass RenderMediaClient? Also, I wonder if ContentRendererClient could have API for instantiating the MediaClient instance instead of RenderThreadImpl being hardcoded to call RenderMediaClient::Initialize? Then the correct client object could be instantiated once and set. https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS File chromecast/public/DEPS (right): https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS... chromecast/public/DEPS:9: "+chromecast/public/media", Doesn't look like you're using this capability?
The CQ bit was checked by servolk@chromium.org 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...
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 servolk@chromium.org 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...
https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... File chromecast/common/media/cast_media_client.cc (right): https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... chromecast/common/media/cast_media_client.cc:9: #define MEDIA_IMPLEMENTATION On 2016/07/27 14:23:17, halliwell wrote: > does this break component build? > > Also, it's a pre-existing problem, but it definitely seems quite ugly how > RenderMediaClient gets instantiated :( > > Could we subclass RenderMediaClient? > Also, I wonder if ContentRendererClient could have API for instantiating the > MediaClient instance instead of RenderThreadImpl being hardcoded to call > RenderMediaClient::Initialize? Then the correct client object could be > instantiated once and set. The most recent trybot runs were all green, including cast_shell trybots, so I think it shouldn't break the build. But to be extra safe we could just duplicate the media::GetMediaClient definition here instead of defining MEDIA_IMPLEMENTATION. The RenderMediaClient unfortunately is not easy subclass - it's internal to content/ and that's enforced by DEPS checks. I'll try following up with content / media folks about this. But I think it will take some time. In the meantime this solution should unblock us for 1.21. https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS File chromecast/public/DEPS (right): https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS... chromecast/public/DEPS:9: "+chromecast/public/media", On 2016/07/27 14:23:17, halliwell wrote: > Doesn't look like you're using this capability? This is actually necessary, without it the deps check would fail: ** Presubmit ERRORS ** You added one or more #includes that violate checkdeps rules. chromecast/public/media_codec_support_shlib.h Illegal include: "chromecast/public/media/decoder_config.h" Because of no rule applying. (and similarly it would still complaint now, with the shorter relative include file name, so keeping this for now). https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/medi... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/medi... chromecast/public/media/decoder_config.h:87: kHEVCMainStillPicture, On 2016/07/27 05:49:39, gfhuang wrote: > this should be added at the bottom, otherwise it will not be backward > compatible. Done. https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/medi... File chromecast/public/media_codec_support_shlib.h (right): https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/medi... chromecast/public/media_codec_support_shlib.h:10: #include "chromecast/public/media/decoder_config.h" On 2016/07/27 05:49:39, gfhuang wrote: > remove "chromecast/public" and use relative path, > otherwise it will cause issues when OEMs include this. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... File chromecast/common/media/cast_media_client.cc (right): https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... chromecast/common/media/cast_media_client.cc:9: #define MEDIA_IMPLEMENTATION On 2016/07/27 18:56:34, servolk wrote: > On 2016/07/27 14:23:17, halliwell wrote: > > does this break component build? > > > > Also, it's a pre-existing problem, but it definitely seems quite ugly how > > RenderMediaClient gets instantiated :( > > > > Could we subclass RenderMediaClient? > > Also, I wonder if ContentRendererClient could have API for instantiating the > > MediaClient instance instead of RenderThreadImpl being hardcoded to call > > RenderMediaClient::Initialize? Then the correct client object could be > > instantiated once and set. > > The most recent trybot runs were all green, including cast_shell trybots, so I > think it shouldn't break the build. But to be extra safe we could just duplicate There is no chromium trybot for component build of cast_shell. slan@ sent an email with instructions on how to build it. > the media::GetMediaClient definition here instead of defining > MEDIA_IMPLEMENTATION. > The RenderMediaClient unfortunately is not easy subclass - it's internal to > content/ and that's enforced by DEPS checks. I'll try following up with content > / media folks about this. But I think it will take some time. In the meantime > this solution should unblock us for 1.21. Ah yes, was forgetting it's not in content 'public'. I agree this solution is fine for unblocking, but would be good to follow up on something cleaner afterwards. https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS File chromecast/public/DEPS (right): https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS... chromecast/public/DEPS:9: "+chromecast/public/media", On 2016/07/27 18:56:34, servolk wrote: > On 2016/07/27 14:23:17, halliwell wrote: > > Doesn't look like you're using this capability? > > This is actually necessary, without it the deps check would fail: > ** Presubmit ERRORS ** > You added one or more #includes that violate checkdeps rules. > chromecast/public/media_codec_support_shlib.h > Illegal include: "chromecast/public/media/decoder_config.h" > Because of no rule applying. > > (and similarly it would still complaint now, with the shorter relative include > file name, so keeping this for now). You can write the include differently, just: #include "decoder_config.h" (see media_pipeline_backend.h for exampl). Then you can revert this DEPS change.
https://codereview.chromium.org/2156193003/diff/240001/chromecast/common/medi... File chromecast/common/media/cast_media_client.cc (right): https://codereview.chromium.org/2156193003/diff/240001/chromecast/common/medi... chromecast/common/media/cast_media_client.cc:68: return content_media_client_->IsSupportedVideoConfig(codec, profile, level); is default impl safe for all OEM devices that we currently have? https://codereview.chromium.org/2156193003/diff/240001/chromecast/public/medi... File chromecast/public/media_codec_support_shlib.h (right): https://codereview.chromium.org/2156193003/diff/240001/chromecast/public/medi... chromecast/public/media_codec_support_shlib.h:36: add comments about what this API is doing (what's the relationship with MediaCodecSupportShlib::IsSupported() ?) and how OEMs supposes to implement this API.
https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... File chromecast/common/media/cast_media_client.cc (right): https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... chromecast/common/media/cast_media_client.cc:9: #define MEDIA_IMPLEMENTATION On 2016/07/27 19:58:48, halliwell wrote: > On 2016/07/27 18:56:34, servolk wrote: > > On 2016/07/27 14:23:17, halliwell wrote: > > > does this break component build? > > > > > > Also, it's a pre-existing problem, but it definitely seems quite ugly how > > > RenderMediaClient gets instantiated :( > > > > > > Could we subclass RenderMediaClient? > > > Also, I wonder if ContentRendererClient could have API for instantiating the > > > MediaClient instance instead of RenderThreadImpl being hardcoded to call > > > RenderMediaClient::Initialize? Then the correct client object could be > > > instantiated once and set. > > > > The most recent trybot runs were all green, including cast_shell trybots, so I > > think it shouldn't break the build. But to be extra safe we could just > duplicate > > There is no chromium trybot for component build of cast_shell. slan@ sent an > email with instructions on how to build it. > > > the media::GetMediaClient definition here instead of defining > > MEDIA_IMPLEMENTATION. > > The RenderMediaClient unfortunately is not easy subclass - it's internal to > > content/ and that's enforced by DEPS checks. I'll try following up with > content > > / media folks about this. But I think it will take some time. In the meantime > > this solution should unblock us for 1.21. > > Ah yes, was forgetting it's not in content 'public'. I agree this solution is > fine for unblocking, but would be good to follow up on something cleaner > afterwards. Ok, I'll try a component build locally. https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS File chromecast/public/DEPS (right): https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS... chromecast/public/DEPS:9: "+chromecast/public/media", On 2016/07/27 19:58:48, halliwell wrote: > On 2016/07/27 18:56:34, servolk wrote: > > On 2016/07/27 14:23:17, halliwell wrote: > > > Doesn't look like you're using this capability? > > > > This is actually necessary, without it the deps check would fail: > > ** Presubmit ERRORS ** > > You added one or more #includes that violate checkdeps rules. > > chromecast/public/media_codec_support_shlib.h > > Illegal include: "chromecast/public/media/decoder_config.h" > > Because of no rule applying. > > > > (and similarly it would still complaint now, with the shorter relative include > > file name, so keeping this for now). > > You can write the include differently, just: > #include "decoder_config.h" > > (see media_pipeline_backend.h for exampl). > > Then you can revert this DEPS change. The difference is that media_pipeline_backend.h is in chromecast/public/media/, i.e. the same directory as decoder_config.h. Whereas media_codec_support_shlib.h is in chromecast/public. When I tried to include just "decoder_config.h" I got build errors. I guess we could add chromecast/public/media/ into include paths in our .gn files. But I'm not sure if that would affect/break vendor builds. Gaofeng, do vendors use GN for their builds?
On 2016/07/27 21:11:38, servolk wrote: > https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... > File chromecast/common/media/cast_media_client.cc (right): > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... > chromecast/common/media/cast_media_client.cc:9: #define MEDIA_IMPLEMENTATION > On 2016/07/27 19:58:48, halliwell wrote: > > On 2016/07/27 18:56:34, servolk wrote: > > > On 2016/07/27 14:23:17, halliwell wrote: > > > > does this break component build? > > > > > > > > Also, it's a pre-existing problem, but it definitely seems quite ugly how > > > > RenderMediaClient gets instantiated :( > > > > > > > > Could we subclass RenderMediaClient? > > > > Also, I wonder if ContentRendererClient could have API for instantiating > the > > > > MediaClient instance instead of RenderThreadImpl being hardcoded to call > > > > RenderMediaClient::Initialize? Then the correct client object could be > > > > instantiated once and set. > > > > > > The most recent trybot runs were all green, including cast_shell trybots, so > I > > > think it shouldn't break the build. But to be extra safe we could just > > duplicate > > > > There is no chromium trybot for component build of cast_shell. slan@ sent an > > email with instructions on how to build it. > > > > > the media::GetMediaClient definition here instead of defining > > > MEDIA_IMPLEMENTATION. > > > The RenderMediaClient unfortunately is not easy subclass - it's internal to > > > content/ and that's enforced by DEPS checks. I'll try following up with > > content > > > / media folks about this. But I think it will take some time. In the > meantime > > > this solution should unblock us for 1.21. > > > > Ah yes, was forgetting it's not in content 'public'. I agree this solution is > > fine for unblocking, but would be good to follow up on something cleaner > > afterwards. > > Ok, I'll try a component build locally. > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS > File chromecast/public/DEPS (right): > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS... > chromecast/public/DEPS:9: "+chromecast/public/media", > On 2016/07/27 19:58:48, halliwell wrote: > > On 2016/07/27 18:56:34, servolk wrote: > > > On 2016/07/27 14:23:17, halliwell wrote: > > > > Doesn't look like you're using this capability? > > > > > > This is actually necessary, without it the deps check would fail: > > > ** Presubmit ERRORS ** > > > You added one or more #includes that violate checkdeps rules. > > > chromecast/public/media_codec_support_shlib.h > > > Illegal include: "chromecast/public/media/decoder_config.h" > > > Because of no rule applying. > > > > > > (and similarly it would still complaint now, with the shorter relative > include > > > file name, so keeping this for now). > > > > You can write the include differently, just: > > #include "decoder_config.h" > > > > (see media_pipeline_backend.h for exampl). > > > > Then you can revert this DEPS change. > > The difference is that media_pipeline_backend.h is in chromecast/public/media/, > i.e. the same directory as decoder_config.h. Whereas media_codec_support_shlib.h > is in chromecast/public. When I tried to include just "decoder_config.h" I got > build errors. > I guess we could add chromecast/public/media/ into include paths in our .gn > files. But I'm not sure if that would affect/break vendor builds. Gaofeng, do > vendors use GN for their builds? public/ interfaces are used outside of GN. so chain-includes inside public/ should be relative to public/
On 2016/07/27 21:13:40, gfhuang wrote: > On 2016/07/27 21:11:38, servolk wrote: > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... > > File chromecast/common/media/cast_media_client.cc (right): > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... > > chromecast/common/media/cast_media_client.cc:9: #define MEDIA_IMPLEMENTATION > > On 2016/07/27 19:58:48, halliwell wrote: > > > On 2016/07/27 18:56:34, servolk wrote: > > > > On 2016/07/27 14:23:17, halliwell wrote: > > > > > does this break component build? > > > > > > > > > > Also, it's a pre-existing problem, but it definitely seems quite ugly > how > > > > > RenderMediaClient gets instantiated :( > > > > > > > > > > Could we subclass RenderMediaClient? > > > > > Also, I wonder if ContentRendererClient could have API for instantiating > > the > > > > > MediaClient instance instead of RenderThreadImpl being hardcoded to call > > > > > RenderMediaClient::Initialize? Then the correct client object could be > > > > > instantiated once and set. > > > > > > > > The most recent trybot runs were all green, including cast_shell trybots, > so > > I > > > > think it shouldn't break the build. But to be extra safe we could just > > > duplicate > > > > > > There is no chromium trybot for component build of cast_shell. slan@ sent > an > > > email with instructions on how to build it. > > > > > > > the media::GetMediaClient definition here instead of defining > > > > MEDIA_IMPLEMENTATION. > > > > The RenderMediaClient unfortunately is not easy subclass - it's internal > to > > > > content/ and that's enforced by DEPS checks. I'll try following up with > > > content > > > > / media folks about this. But I think it will take some time. In the > > meantime > > > > this solution should unblock us for 1.21. > > > > > > Ah yes, was forgetting it's not in content 'public'. I agree this solution > is > > > fine for unblocking, but would be good to follow up on something cleaner > > > afterwards. > > > > Ok, I'll try a component build locally. > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS > > File chromecast/public/DEPS (right): > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS... > > chromecast/public/DEPS:9: "+chromecast/public/media", > > On 2016/07/27 19:58:48, halliwell wrote: > > > On 2016/07/27 18:56:34, servolk wrote: > > > > On 2016/07/27 14:23:17, halliwell wrote: > > > > > Doesn't look like you're using this capability? > > > > > > > > This is actually necessary, without it the deps check would fail: > > > > ** Presubmit ERRORS ** > > > > You added one or more #includes that violate checkdeps rules. > > > > chromecast/public/media_codec_support_shlib.h > > > > Illegal include: "chromecast/public/media/decoder_config.h" > > > > Because of no rule applying. > > > > > > > > (and similarly it would still complaint now, with the shorter relative > > include > > > > file name, so keeping this for now). > > > > > > You can write the include differently, just: > > > #include "decoder_config.h" > > > > > > (see media_pipeline_backend.h for exampl). > > > > > > Then you can revert this DEPS change. > > > > The difference is that media_pipeline_backend.h is in > chromecast/public/media/, > > i.e. the same directory as decoder_config.h. Whereas > media_codec_support_shlib.h > > is in chromecast/public. When I tried to include just "decoder_config.h" I got > > build errors. > > I guess we could add chromecast/public/media/ into include paths in our .gn > > files. But I'm not sure if that would affect/break vendor builds. Gaofeng, do > > vendors use GN for their builds? Ah I see. In that case it might be ok. Having said that, you could just put your new code in public/media since it's media-related? I think it's good to put a class in a file with the same name in any case.
The CQ bit was checked by servolk@chromium.org 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/07/27 21:54:37, halliwell wrote: > On 2016/07/27 21:13:40, gfhuang wrote: > > On 2016/07/27 21:11:38, servolk wrote: > > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... > > > File chromecast/common/media/cast_media_client.cc (right): > > > > > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... > > > chromecast/common/media/cast_media_client.cc:9: #define MEDIA_IMPLEMENTATION > > > On 2016/07/27 19:58:48, halliwell wrote: > > > > On 2016/07/27 18:56:34, servolk wrote: > > > > > On 2016/07/27 14:23:17, halliwell wrote: > > > > > > does this break component build? > > > > > > > > > > > > Also, it's a pre-existing problem, but it definitely seems quite ugly > > how > > > > > > RenderMediaClient gets instantiated :( > > > > > > > > > > > > Could we subclass RenderMediaClient? > > > > > > Also, I wonder if ContentRendererClient could have API for > instantiating > > > the > > > > > > MediaClient instance instead of RenderThreadImpl being hardcoded to > call > > > > > > RenderMediaClient::Initialize? Then the correct client object could > be > > > > > > instantiated once and set. > > > > > > > > > > The most recent trybot runs were all green, including cast_shell > trybots, > > so > > > I > > > > > think it shouldn't break the build. But to be extra safe we could just > > > > duplicate > > > > > > > > There is no chromium trybot for component build of cast_shell. slan@ sent > > an > > > > email with instructions on how to build it. > > > > > > > > > the media::GetMediaClient definition here instead of defining > > > > > MEDIA_IMPLEMENTATION. > > > > > The RenderMediaClient unfortunately is not easy subclass - it's internal > > to > > > > > content/ and that's enforced by DEPS checks. I'll try following up with > > > > content > > > > > / media folks about this. But I think it will take some time. In the > > > meantime > > > > > this solution should unblock us for 1.21. > > > > > > > > Ah yes, was forgetting it's not in content 'public'. I agree this > solution > > is > > > > fine for unblocking, but would be good to follow up on something cleaner > > > > afterwards. > > > > > > Ok, I'll try a component build locally. > > > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS > > > File chromecast/public/DEPS (right): > > > > > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS... > > > chromecast/public/DEPS:9: "+chromecast/public/media", > > > On 2016/07/27 19:58:48, halliwell wrote: > > > > On 2016/07/27 18:56:34, servolk wrote: > > > > > On 2016/07/27 14:23:17, halliwell wrote: > > > > > > Doesn't look like you're using this capability? > > > > > > > > > > This is actually necessary, without it the deps check would fail: > > > > > ** Presubmit ERRORS ** > > > > > You added one or more #includes that violate checkdeps rules. > > > > > chromecast/public/media_codec_support_shlib.h > > > > > Illegal include: "chromecast/public/media/decoder_config.h" > > > > > Because of no rule applying. > > > > > > > > > > (and similarly it would still complaint now, with the shorter relative > > > include > > > > > file name, so keeping this for now). > > > > > > > > You can write the include differently, just: > > > > #include "decoder_config.h" > > > > > > > > (see media_pipeline_backend.h for exampl). > > > > > > > > Then you can revert this DEPS change. > > > > > > The difference is that media_pipeline_backend.h is in > > chromecast/public/media/, > > > i.e. the same directory as decoder_config.h. Whereas > > media_codec_support_shlib.h > > > is in chromecast/public. When I tried to include just "decoder_config.h" I > got > > > build errors. > > > I guess we could add chromecast/public/media/ into include paths in our .gn > > > files. But I'm not sure if that would affect/break vendor builds. Gaofeng, > do > > > vendors use GN for their builds? > > > Ah I see. In that case it might be ok. Having said that, you could just put > your new code in public/media since it's media-related? I think it's good to > put a class in a file with the same name in any case. Yeah, I just uploaded a new patchset that moves the MediaCapShlib interface to a new header under public/media dir :)
On 2016/07/27 21:55:55, servolk wrote: > On 2016/07/27 21:54:37, halliwell wrote: > > On 2016/07/27 21:13:40, gfhuang wrote: > > > On 2016/07/27 21:11:38, servolk wrote: > > > > > > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... > > > > File chromecast/common/media/cast_media_client.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... > > > > chromecast/common/media/cast_media_client.cc:9: #define > MEDIA_IMPLEMENTATION > > > > On 2016/07/27 19:58:48, halliwell wrote: > > > > > On 2016/07/27 18:56:34, servolk wrote: > > > > > > On 2016/07/27 14:23:17, halliwell wrote: > > > > > > > does this break component build? > > > > > > > > > > > > > > Also, it's a pre-existing problem, but it definitely seems quite > ugly > > > how > > > > > > > RenderMediaClient gets instantiated :( > > > > > > > > > > > > > > Could we subclass RenderMediaClient? > > > > > > > Also, I wonder if ContentRendererClient could have API for > > instantiating > > > > the > > > > > > > MediaClient instance instead of RenderThreadImpl being hardcoded to > > call > > > > > > > RenderMediaClient::Initialize? Then the correct client object could > > be > > > > > > > instantiated once and set. > > > > > > > > > > > > The most recent trybot runs were all green, including cast_shell > > trybots, > > > so > > > > I > > > > > > think it shouldn't break the build. But to be extra safe we could just > > > > > duplicate > > > > > > > > > > There is no chromium trybot for component build of cast_shell. slan@ > sent > > > an > > > > > email with instructions on how to build it. > > > > > > > > > > > the media::GetMediaClient definition here instead of defining > > > > > > MEDIA_IMPLEMENTATION. > > > > > > The RenderMediaClient unfortunately is not easy subclass - it's > internal > > > to > > > > > > content/ and that's enforced by DEPS checks. I'll try following up > with > > > > > content > > > > > > / media folks about this. But I think it will take some time. In the > > > > meantime > > > > > > this solution should unblock us for 1.21. > > > > > > > > > > Ah yes, was forgetting it's not in content 'public'. I agree this > > solution > > > is > > > > > fine for unblocking, but would be good to follow up on something cleaner > > > > > afterwards. > > > > > > > > Ok, I'll try a component build locally. > > > > > > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS > > > > File chromecast/public/DEPS (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS... > > > > chromecast/public/DEPS:9: "+chromecast/public/media", > > > > On 2016/07/27 19:58:48, halliwell wrote: > > > > > On 2016/07/27 18:56:34, servolk wrote: > > > > > > On 2016/07/27 14:23:17, halliwell wrote: > > > > > > > Doesn't look like you're using this capability? > > > > > > > > > > > > This is actually necessary, without it the deps check would fail: > > > > > > ** Presubmit ERRORS ** > > > > > > You added one or more #includes that violate checkdeps rules. > > > > > > chromecast/public/media_codec_support_shlib.h > > > > > > Illegal include: "chromecast/public/media/decoder_config.h" > > > > > > Because of no rule applying. > > > > > > > > > > > > (and similarly it would still complaint now, with the shorter relative > > > > include > > > > > > file name, so keeping this for now). > > > > > > > > > > You can write the include differently, just: > > > > > #include "decoder_config.h" > > > > > > > > > > (see media_pipeline_backend.h for exampl). > > > > > > > > > > Then you can revert this DEPS change. > > > > > > > > The difference is that media_pipeline_backend.h is in > > > chromecast/public/media/, > > > > i.e. the same directory as decoder_config.h. Whereas > > > media_codec_support_shlib.h > > > > is in chromecast/public. When I tried to include just "decoder_config.h" I > > got > > > > build errors. > > > > I guess we could add chromecast/public/media/ into include paths in our > .gn > > > > files. But I'm not sure if that would affect/break vendor builds. Gaofeng, > > do > > > > vendors use GN for their builds? > > > > > > Ah I see. In that case it might be ok. Having said that, you could just put > > your new code in public/media since it's media-related? I think it's good to > > put a class in a file with the same name in any case. > > Yeah, I just uploaded a new patchset that moves the MediaCapShlib interface to a > new header under public/media dir :) Ok cool, this looks almost there, just need to consider commenting the new interface. Comments in these headers are our primary documentation for vendors so it needs to be more than our normal code :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org 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...
https://codereview.chromium.org/2156193003/diff/240001/chromecast/common/medi... File chromecast/common/media/cast_media_client.cc (right): https://codereview.chromium.org/2156193003/diff/240001/chromecast/common/medi... chromecast/common/media/cast_media_client.cc:68: return content_media_client_->IsSupportedVideoConfig(codec, profile, level); On 2016/07/27 20:13:57, gfhuang wrote: > is default impl safe for all OEM devices that we currently have? Good point, I guess we can't really guarantee that the default content::RenderMediaClient is safe, but we can provide our own safe default implementation here instead. I've done that in the latest patchset. https://codereview.chromium.org/2156193003/diff/240001/chromecast/public/medi... File chromecast/public/media_codec_support_shlib.h (right): https://codereview.chromium.org/2156193003/diff/240001/chromecast/public/medi... chromecast/public/media_codec_support_shlib.h:36: On 2016/07/27 20:13:58, gfhuang wrote: > add comments about what this API is doing > (what's the relationship with MediaCodecSupportShlib::IsSupported() ?) > > and how OEMs supposes to implement this API. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/27 21:58:51, halliwell wrote: > On 2016/07/27 21:55:55, servolk wrote: > > On 2016/07/27 21:54:37, halliwell wrote: > > > On 2016/07/27 21:13:40, gfhuang wrote: > > > > On 2016/07/27 21:11:38, servolk wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... > > > > > File chromecast/common/media/cast_media_client.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/common/medi... > > > > > chromecast/common/media/cast_media_client.cc:9: #define > > MEDIA_IMPLEMENTATION > > > > > On 2016/07/27 19:58:48, halliwell wrote: > > > > > > On 2016/07/27 18:56:34, servolk wrote: > > > > > > > On 2016/07/27 14:23:17, halliwell wrote: > > > > > > > > does this break component build? > > > > > > > > > > > > > > > > Also, it's a pre-existing problem, but it definitely seems quite > > ugly > > > > how > > > > > > > > RenderMediaClient gets instantiated :( > > > > > > > > > > > > > > > > Could we subclass RenderMediaClient? > > > > > > > > Also, I wonder if ContentRendererClient could have API for > > > instantiating > > > > > the > > > > > > > > MediaClient instance instead of RenderThreadImpl being hardcoded > to > > > call > > > > > > > > RenderMediaClient::Initialize? Then the correct client object > could > > > be > > > > > > > > instantiated once and set. > > > > > > > > > > > > > > The most recent trybot runs were all green, including cast_shell > > > trybots, > > > > so > > > > > I > > > > > > > think it shouldn't break the build. But to be extra safe we could > just > > > > > > duplicate > > > > > > > > > > > > There is no chromium trybot for component build of cast_shell. slan@ > > sent > > > > an > > > > > > email with instructions on how to build it. > > > > > > > > > > > > > the media::GetMediaClient definition here instead of defining > > > > > > > MEDIA_IMPLEMENTATION. > > > > > > > The RenderMediaClient unfortunately is not easy subclass - it's > > internal > > > > to > > > > > > > content/ and that's enforced by DEPS checks. I'll try following up > > with > > > > > > content > > > > > > > / media folks about this. But I think it will take some time. In the > > > > > meantime > > > > > > > this solution should unblock us for 1.21. > > > > > > > > > > > > Ah yes, was forgetting it's not in content 'public'. I agree this > > > solution > > > > is > > > > > > fine for unblocking, but would be good to follow up on something > cleaner > > > > > > afterwards. > > > > > > > > > > Ok, I'll try a component build locally. > > > > > > > > > > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS > > > > > File chromecast/public/DEPS (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2156193003/diff/220001/chromecast/public/DEPS... > > > > > chromecast/public/DEPS:9: "+chromecast/public/media", > > > > > On 2016/07/27 19:58:48, halliwell wrote: > > > > > > On 2016/07/27 18:56:34, servolk wrote: > > > > > > > On 2016/07/27 14:23:17, halliwell wrote: > > > > > > > > Doesn't look like you're using this capability? > > > > > > > > > > > > > > This is actually necessary, without it the deps check would fail: > > > > > > > ** Presubmit ERRORS ** > > > > > > > You added one or more #includes that violate checkdeps rules. > > > > > > > chromecast/public/media_codec_support_shlib.h > > > > > > > Illegal include: "chromecast/public/media/decoder_config.h" > > > > > > > Because of no rule applying. > > > > > > > > > > > > > > (and similarly it would still complaint now, with the shorter > relative > > > > > include > > > > > > > file name, so keeping this for now). > > > > > > > > > > > > You can write the include differently, just: > > > > > > #include "decoder_config.h" > > > > > > > > > > > > (see media_pipeline_backend.h for exampl). > > > > > > > > > > > > Then you can revert this DEPS change. > > > > > > > > > > The difference is that media_pipeline_backend.h is in > > > > chromecast/public/media/, > > > > > i.e. the same directory as decoder_config.h. Whereas > > > > media_codec_support_shlib.h > > > > > is in chromecast/public. When I tried to include just "decoder_config.h" > I > > > got > > > > > build errors. > > > > > I guess we could add chromecast/public/media/ into include paths in our > > .gn > > > > > files. But I'm not sure if that would affect/break vendor builds. > Gaofeng, > > > do > > > > > vendors use GN for their builds? > > > > > > > > > Ah I see. In that case it might be ok. Having said that, you could just > put > > > your new code in public/media since it's media-related? I think it's good > to > > > put a class in a file with the same name in any case. > > > > Yeah, I just uploaded a new patchset that moves the MediaCapShlib interface to > a > > new header under public/media dir :) > > Ok cool, this looks almost there, just need to consider commenting the new > interface. Comments in these headers are our primary documentation for vendors > so it needs to be more than our normal code :) Ok, I've added the comments in the latest patchset and tried component build in the Chromecast tree and it was fine, since GetMediaClient is actually marked as MEDIA_EXPORT in the original header anyway, so it's always accessible (and exported from media shared lib for component build). So I believe this should be good to go now.
https://codereview.chromium.org/2156193003/diff/300001/chromecast/public/medi... File chromecast/public/media/media_capabilities_shlib.h (right): https://codereview.chromium.org/2156193003/diff/300001/chromecast/public/medi... chromecast/public/media/media_capabilities_shlib.h:22: // level value corresponds to level_idc defined in the H.264 / HEVC standard. Is this new API intend to deprecate the other old API?
https://codereview.chromium.org/2156193003/diff/300001/chromecast/public/medi... File chromecast/public/media/media_capabilities_shlib.h (right): https://codereview.chromium.org/2156193003/diff/300001/chromecast/public/medi... chromecast/public/media/media_capabilities_shlib.h:22: // level value corresponds to level_idc defined in the H.264 / HEVC standard. On 2016/07/28 00:38:07, gfhuang wrote: > Is this new API intend to deprecate the other old API? > > Yes, eventually. But for now we will support both for backward compatibility, and also due to the fact that this new API is only for video and doesn't handle cases of multi-channel audio formats. The new API is better aligned with what upstream media folks want and also makes it easier to express more fine-grained platform capabilities via profile/level combination.
On 2016/07/28 00:49:20, servolk wrote: > https://codereview.chromium.org/2156193003/diff/300001/chromecast/public/medi... > File chromecast/public/media/media_capabilities_shlib.h (right): > > https://codereview.chromium.org/2156193003/diff/300001/chromecast/public/medi... > chromecast/public/media/media_capabilities_shlib.h:22: // level value > corresponds to level_idc defined in the H.264 / HEVC standard. > On 2016/07/28 00:38:07, gfhuang wrote: > > Is this new API intend to deprecate the other old API? > > > > > > Yes, eventually. But for now we will support both for backward compatibility, > and also due to the fact that this new API is only for video and doesn't handle > cases of multi-channel audio formats. The new API is better aligned with what > upstream media folks want and also makes it easier to express more fine-grained > platform capabilities via profile/level combination. lgtm
public/ lgtm
public/ lgtm
The CQ bit was checked by servolk@chromium.org
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.
Description was changed from ========== [Chromecast] Add a custom CastMediaClient for Chromecast It will forward MediaClient::IsSupportedVideoConfig invocations to a shared library implementation, which will allow us to have different codecs supported on different platforms. BUG=477103 ========== to ========== [Chromecast] Add a custom CastMediaClient for Chromecast It will forward MediaClient::IsSupportedVideoConfig invocations to a shared library implementation, which will allow us to have different codecs supported on different platforms. BUG=477103 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Add a custom CastMediaClient for Chromecast It will forward MediaClient::IsSupportedVideoConfig invocations to a shared library implementation, which will allow us to have different codecs supported on different platforms. BUG=477103 ========== to ========== [Chromecast] Add a custom CastMediaClient for Chromecast It will forward MediaClient::IsSupportedVideoConfig invocations to a shared library implementation, which will allow us to have different codecs supported on different platforms. BUG=477103 Committed: https://crrev.com/29ce77106c738a40237b4a1715f953913e493413 Cr-Commit-Position: refs/heads/master@{#408300} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/29ce77106c738a40237b4a1715f953913e493413 Cr-Commit-Position: refs/heads/master@{#408300} |
