Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1946153002:
Fix V4L2 device failed to create on veyron board (Closed)
|
Created:
4 years, 7 months ago by kcwu Modified:
4 years, 7 months ago CC:
chromium-reviews, jam, rickyz+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, jln+watch_chromium.org, piman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix V4L2 device failed to create on veyron board
This was broken since CL 6e70beb8, gpu & media refactoring.
This CL modified the build rule, so dependencies of media/gpu will define USE_LIBV4L2 automatically, which is needed by
content/common/sandbox_linux/bpf_gpu_policy_linux.cc.
TEST=play video on veyron and VDA works
BUG=608809, 608838, 586386
Committed: https://crrev.com/152f266aafe81cf9ef79aec4b57d9e80cea4fef0
Cr-Commit-Position: refs/heads/master@{#391911}
Patch Set 1 #
Total comments: 1
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 5
Patch Set 5 : public_configs in media/gpu/ipc/client #
Messages
Total messages: 28 (12 generated)
Description was changed from ========== Fix V4L2 device failed to create on veyron board TEST=play video on veyron and VDA works BUG=608809,608838 ========== to ========== Fix V4L2 device failed to create on veyron board This was broken since CL 6e70beb8, gpu & media refactoring. TEST=play video on veyron and VDA works BUG=608809,608838,586386 ==========
kcwu@chromium.org changed reviewers: + markdittmer@chromium.org, wuchengli@chromium.org
kcwu@chromium.org changed reviewers: + piman@chromium.org
lgtm
One possible issue. I recently hit a bot failure when a static method wasn't exported (even though the class was exported). Not sure whether the issue was platform-specific and/or whether platforms affected by this change require it. https://codereview.chromium.org/1946153002/diff/1/media/gpu/v4l2_device.h File media/gpu/v4l2_device.h (right): https://codereview.chromium.org/1946153002/diff/1/media/gpu/v4l2_device.h#new... media/gpu/v4l2_device.h:55: static bool IsUsingLibV4L2(); Does this also need MEDIA_GPU_EXPORT?
lgtm
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
I changed the approach, please take another look. I found my previous patch breaks if (is_linux && !use_v4l2_codec). Since v4l2 codec is guard by USE_V4L2_CODEC, it doesn't make sense to query v4l plugin status if USE_V4L2_CODEC is undefined. So I use another approach, which modifying build rules to define USE_LIBV4L2 for dependencies of media/gpu.
LGTM
kcwu@chromium.org changed reviewers: + xhwang@chromium.org - piman@chromium.org
+xhwang, media/ owner review, please. Move piman to cc list since content/ code is no longer touched.
xhwang@ I'm the owner of media/gpu and I already reviewed media/gpu/BUILD.gn. You can review only media/media_gpu.gypi if you want. https://codereview.chromium.org/1946153002/diff/120001/media/media_gpu.gypi File media/media_gpu.gypi (right): https://codereview.chromium.org/1946153002/diff/120001/media/media_gpu.gypi#n... media/media_gpu.gypi:127: 'direct_dependent_settings': { FYI. content_common.gypi depends on media.gyp:media_gpu (#1). media_gpu target includes media_gpu.gypi (#2). content_common.gypi needs USE_LIBV4L2 because content/common/sandbox_linux/bpf_gpu_policy_linux.cc needs it (#3). #1: https://code.google.com/p/chromium/codesearch#chromium/src/content/content_co... #2: https://code.google.com/p/chromium/codesearch#chromium/src/media/media.gyp&q=... #3: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/san...
https://codereview.chromium.org/1946153002/diff/120001/media/gpu/BUILD.gn File media/gpu/BUILD.gn (left): https://codereview.chromium.org/1946153002/diff/120001/media/gpu/BUILD.gn#old... media/gpu/BUILD.gn:238: defines += [ "USE_LIBV4L2" ] FYI. all_dependent_configs applies the config to the target itself. So we don't need this. https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/quick_s... https://codereview.chromium.org/1946153002/diff/120001/media/gpu/BUILD.gn File media/gpu/BUILD.gn (right): https://codereview.chromium.org/1946153002/diff/120001/media/gpu/BUILD.gn#new... media/gpu/BUILD.gn:177: all_dependent_configs = [ ":gpu_config" ] FYI. All targets that transitively depend on the current one will get gpu_config. https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/quick_s...
Description was changed from ========== Fix V4L2 device failed to create on veyron board This was broken since CL 6e70beb8, gpu & media refactoring. TEST=play video on veyron and VDA works BUG=608809,608838,586386 ========== to ========== Fix V4L2 device failed to create on veyron board This was broken since CL 6e70beb8, gpu & media refactoring. This CL modified the build rule, so dependencies of media/gpu will define USE_LIBV4L2 automatically, which is needed by content/common/sandbox_linux/bpf_gpu_policy_linux.cc. TEST=play video on veyron and VDA works BUG=608809,608838,586386 ==========
https://codereview.chromium.org/1946153002/diff/120001/media/gpu/BUILD.gn File media/gpu/BUILD.gn (right): https://codereview.chromium.org/1946153002/diff/120001/media/gpu/BUILD.gn#new... media/gpu/BUILD.gn:177: all_dependent_configs = [ ":gpu_config" ] On 2016/05/05 14:44:05, wuchengli wrote: > FYI. All targets that transitively depend on the current one will get > gpu_config. > > https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/quick_s... Will public_configs work for your use case? We try to avoid using all_dependent_configs as much as possible.
On 2016/05/05 14:36:49, wuchengli wrote: > xhwang@ I'm the owner of media/gpu and I already reviewed media/gpu/BUILD.gn. > You can review only media/media_gpu.gypi if you want. We should have a per-file owner (the same as media/gpu/OWNERS) for media_gpu.gypi
https://codereview.chromium.org/1946153002/diff/120001/media/gpu/BUILD.gn File media/gpu/BUILD.gn (right): https://codereview.chromium.org/1946153002/diff/120001/media/gpu/BUILD.gn#new... media/gpu/BUILD.gn:177: all_dependent_configs = [ ":gpu_config" ] On 2016/05/05 16:59:13, xhwang wrote: > On 2016/05/05 14:44:05, wuchengli wrote: > > FYI. All targets that transitively depend on the current one will get > > gpu_config. > > > > > https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/quick_s... > > Will public_configs work for your use case? We try to avoid using > all_dependent_configs as much as possible. content/common doesn't depend on media/gpu directly. Should I add deps to content/common just for getting macro defined?
LGTM
The CQ bit was checked by kcwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, wuchengli@chromium.org Link to the patchset: https://codereview.chromium.org/1946153002/#ps140001 (title: "public_configs in media/gpu/ipc/client")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1946153002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1946153002/140001
Message was sent while issue was closed.
Description was changed from ========== Fix V4L2 device failed to create on veyron board This was broken since CL 6e70beb8, gpu & media refactoring. This CL modified the build rule, so dependencies of media/gpu will define USE_LIBV4L2 automatically, which is needed by content/common/sandbox_linux/bpf_gpu_policy_linux.cc. TEST=play video on veyron and VDA works BUG=608809,608838,586386 ========== to ========== Fix V4L2 device failed to create on veyron board This was broken since CL 6e70beb8, gpu & media refactoring. This CL modified the build rule, so dependencies of media/gpu will define USE_LIBV4L2 automatically, which is needed by content/common/sandbox_linux/bpf_gpu_policy_linux.cc. TEST=play video on veyron and VDA works BUG=608809,608838,586386 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Fix V4L2 device failed to create on veyron board This was broken since CL 6e70beb8, gpu & media refactoring. This CL modified the build rule, so dependencies of media/gpu will define USE_LIBV4L2 automatically, which is needed by content/common/sandbox_linux/bpf_gpu_policy_linux.cc. TEST=play video on veyron and VDA works BUG=608809,608838,586386 ========== to ========== Fix V4L2 device failed to create on veyron board This was broken since CL 6e70beb8, gpu & media refactoring. This CL modified the build rule, so dependencies of media/gpu will define USE_LIBV4L2 automatically, which is needed by content/common/sandbox_linux/bpf_gpu_policy_linux.cc. TEST=play video on veyron and VDA works BUG=608809,608838,586386 Committed: https://crrev.com/152f266aafe81cf9ef79aec4b57d9e80cea4fef0 Cr-Commit-Position: refs/heads/master@{#391911} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/152f266aafe81cf9ef79aec4b57d9e80cea4fef0 Cr-Commit-Position: refs/heads/master@{#391911} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
