Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1882373004:
Migrate content/common/gpu/media code to media/gpu (Closed)
|
Created:
4 years, 8 months ago by Mark Dittmer Modified:
4 years, 7 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, rickyz+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMigrate content/common/gpu/media code to media/gpu
This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details.
BUG=586386
Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81
Cr-Commit-Position: refs/heads/master@{#390896}
Committed: https://crrev.com/0f54920173bda1d13e9c89c51a1995a8542c5648
Cr-Commit-Position: refs/heads/master@{#390897}
Committed: https://crrev.com/9204aefa88c2a7af21e288dd3f1a5b8db468002a
Cr-Commit-Position: refs/heads/master@{#390908}
Patch Set 1 #Patch Set 2 : Delete remaining files in content/common/gpu/media and fix up a couple gn-todos #Patch Set 3 : git cl format #Patch Set 4 : rebase #Patch Set 5 : Switch to MEDIA_GPU_EXPORT in media/gpu #Patch Set 6 : content/gpu dep on media/gpu/ipc/service #Patch Set 7 : Fix prefix to content references in content_gpu.gypi #
Total comments: 2
Patch Set 8 : Fix several build bot issues #Patch Set 9 : rebase #Patch Set 10 : Fix up mesa headers dep #Patch Set 11 : Fix up several bot-identified issues #Patch Set 12 : rebase #Patch Set 13 : Add required vt_stubs header directory generated bt gpu/media to gpu/media/ipc/service #Patch Set 14 : Fix several more bot-identified build issues #
Total comments: 19
Patch Set 15 : Duplicate vt_stubs for content/common and media/gpu/ipc/service on mac #Patch Set 16 : rebase #Patch Set 17 : rebase #Patch Set 18 : Fix namespace qualification and common media/gpu stubs location (gyp) #Patch Set 19 : Fix android unittest dep (gn) #Patch Set 20 : Update media/gpu visibility list to provide access to test code #Patch Set 21 : Switch ID3D11VideoDevice identification from IID_... to __uuidof(...) #Patch Set 22 : rebase #Patch Set 23 : Fix namespace qualifier on g_test_import #Patch Set 24 : Rebase and drop duplicate vt_stubs generation #Patch Set 25 : Fix content/test/BUILD.gn rebase; add //media/gpu to content/renderer deps; sync media/gpu-related … #Patch Set 26 : Squash and rebase #
Total comments: 1
Patch Set 27 : Rebase and refresh from relevant content/common media-related changes #Patch Set 28 : Fix several bot-identified issues #Patch Set 29 : Fix up some broken includes #Patch Set 30 : Add //media/gpu as dep for //media/gpu:*_unittest #Patch Set 31 : Add missing gfx_ipc_geometry dep to media/gpu/ipc/common #Patch Set 32 : Add missing dep on media_gpu to content/gpu GN build #Patch Set 33 : Add missing deps and export AVDASurfaceTracker #Patch Set 34 : Fix media/gpu visibility #Patch Set 35 : git cl format media #Patch Set 36 : Address comments from posciak@ #Patch Set 37 : Revert git-cl-format-breaking changes #Patch Set 38 : Add X11 headers to vaapi_tfp_picture.cc #Patch Set 39 : Add X11 headers to vaapi_tfp_picture.cc #Patch Set 40 : Add x11 config to x11 ChromeOS build #Patch Set 41 : rebase #Messages
Total messages: 139 (44 generated)
Description was changed from ========== Migrate content/common/gpu/media code to media/gpu BUG=586386 ========== to ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 ==========
Delete remaining files in content/common/gpu/media and fix up a couple gn-todos
git cl format
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/20001
markdittmer@chromium.org changed reviewers: + agrieve@chromium.org, piman@chromium.org, xhwang@chromium.org
Reviewers, PTAL. agrieve@ -- //build/** piman@ -- //content/** xhwang@ -- //media/**
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Relevant to media/gpu review:
"""
You added one or more references to the base::Time class and/or one
of its member functions (or base::Clock/DefaultClock). In media
code, it is rarely correct to use a clock susceptible to time skew!
Instead, could you use base::TimeTicks to
track the passage of
real-world time?
media/gpu/android_video_encode_accelerator.cc:249
base::MakeTuple(frame, force_keyframe,
base::Time::Now()));
media/gpu/android_video_encode_accelerator.cc:354
base::Time::Now() -
base::get<2>(input));
media/gpu/android_video_encode_accelerator.h:89
base::Tuple<scoped_refptr<media::VideoFrame>, bool, base::Time>>
"""
LGTM for content/. Thanks! On 2016/04/14 19:37:52, Mark Dittmer wrote: > Relevant to media/gpu review: > > """ > You added one or more references to the base::Time class and/or one > > of its member functions (or base::Clock/DefaultClock). In media > > code, it is rarely correct to use a clock susceptible to time skew! > Instead, could you use base::TimeTicks to > track the passage of > real-world time? > > > media/gpu/android_video_encode_accelerator.cc:249 > base::MakeTuple(frame, force_keyframe, > base::Time::Now())); > media/gpu/android_video_encode_accelerator.cc:354 > base::Time::Now() - > base::get<2>(input)); > > media/gpu/android_video_encode_accelerator.h:89 > > base::Tuple<scoped_refptr<media::VideoFrame>, bool, base::Time>> > """ If these need a fix (I have no idea), let's do it in a separate CL, to keep this without semantic changes (it'll be too hard to revert later).
On 2016/04/14 20:12:29, piman wrote: > If these need a fix (I have no idea), let's do it in a separate CL, to keep this > without semantic changes (it'll be too hard to revert later). I agree. What do you think, xhwang@?
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/20001
rebase
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/30001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/30001
LGTM This CL should be move-only. Can you add a TODO for the warning?
On 2016/04/14 20:33:15, xhwang wrote: > LGTM > > This CL should be move-only. > > Can you add a TODO for the warning? not lgtm Actually, you should not use MEDIA_EXPORT for targets other than "media". Please either define a new *_EXPORT, e.g. MEDIA_BLINK_EXPORT for the "gpu" target, or just use a source_set instead of a component. I'll take a look again after this change. Sorry for not finding it earlier.
On 2016/04/14 20:37:06, xhwang wrote: > On 2016/04/14 20:33:15, xhwang wrote: > > LGTM > > > > This CL should be move-only. > > > > Can you add a TODO for the warning? > > not lgtm > > Actually, you should not use MEDIA_EXPORT for targets other than "media". > > Please either define a new *_EXPORT, e.g. MEDIA_BLINK_EXPORT for the "gpu" > target, or just use a source_set instead of a component. > > I'll take a look again after this change. Sorry for not finding it earlier. Please also add a deps rule to make sure we never use #include "media/base/media_export.h" in media/gpu, similar to this: https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/DEPS&l=8
On 2016/04/14 20:37:06, xhwang wrote: > On 2016/04/14 20:33:15, xhwang wrote: > > LGTM > > > > This CL should be move-only. > > > > Can you add a TODO for the warning? > > not lgtm > > Actually, you should not use MEDIA_EXPORT for targets other than "media". > > Please either define a new *_EXPORT, e.g. MEDIA_BLINK_EXPORT for the "gpu" > target, or just use a source_set instead of a component. > > I'll take a look again after this change. Sorry for not finding it earlier. build/ lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
I would like to build and test this CL on Chrome OS before it's submitted please, as currently Chrome CQ does not compile or test a lot of Chrome OS code being moved with this change. Unfortunately it looks like "patch set is too large to download". Is there any other way I could get the patch please? Thanks.
Switch to MEDIA_GPU_EXPORT in media/gpu
On 2016/04/15 12:49:58, Mark Dittmer wrote: > Switch to MEDIA_GPU_EXPORT in media/gpu xhwang@, PTAL.
content/gpu dep on media/gpu/ipc/service
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/50001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Fix prefix to content references in content_gpu.gypi
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
lgtm % tiny nit https://codereview.chromium.org/1882373004/diff/60001/media/gpu/media_gpu_exp... File media/gpu/media_gpu_export.h (right): https://codereview.chromium.org/1882373004/diff/60001/media/gpu/media_gpu_exp... media/gpu/media_gpu_export.h:8: // Define MEDIA_GPU_EXPORT so that functionality implemented by the Media module media gpu module?
Fix several build bot issues
rebase
Fix up mesa headers dep
Fix up several bot-identified issues
rebase
Add required vt_stubs header directory generated bt gpu/media to gpu/media/ipc/service
https://codereview.chromium.org/1882373004/diff/60001/media/gpu/media_gpu_exp... File media/gpu/media_gpu_export.h (right): https://codereview.chromium.org/1882373004/diff/60001/media/gpu/media_gpu_exp... media/gpu/media_gpu_export.h:8: // Define MEDIA_GPU_EXPORT so that functionality implemented by the Media module On 2016/04/15 15:54:45, xhwang (OOO Apr 15) wrote: > media gpu module? Done.
Fix several more bot-identified build issues
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/120001
piman@ PTAL at ui/gl/gl_bindings.h https://codereview.chromium.org/1882373004/diff/120001/ui/gl/gl_bindings.h File ui/gl/gl_bindings.h (right): https://codereview.chromium.org/1882373004/diff/120001/ui/gl/gl_bindings.h#ne... ui/gl/gl_bindings.h:13: // GL headers may include inttypes.h and so we need to ensure that piman@, agl@ helped me debug this issue [1] and recommended I put the change here, where the GL headers causing the problem were initially pulled in. PTAL. [1] https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...
LGTM. It seems it would be more appropriate to have __STDC_FORMAT_MACROS be set in the gyp/gn to ensure consistency, but maybe it has some other side effect.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
markdittmer@chromium.org changed reviewers: + ben@chromium.org, sadrul@chromium.org
+ben@ and +sadrul@ for DEPS owners review. PTAL at new DEPS: ben@ -- third_party sadrul@ -- ui
posciak@chromium.org changed reviewers: + posciak@chromium.org
Built and tested on Chrome OS and things appear to be working fine. As for the CL, personally I feel that automated code reformattings made it significantly less readable in some cases in VDAs. Especially logging (LOG(), DVLOG() etc.) messages became less readable, and they were specifically, manually formatted in a way to make them easy to read while debugging. Would it be possible to revert these changes please? On the other hand, we have quite a lot of media:: left, which should not be needed now that everything is in media namespace... https://codereview.chromium.org/1882373004/diff/120001/media/gpu/h264_decoder.cc File media/gpu/h264_decoder.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder.cc:1315: // else fallthrough I believe the indent here was correct. https://codereview.chromium.org/1882373004/diff/120001/media/gpu/ipc/service/... File media/gpu/ipc/service/gpu_video_decode_accelerator_factory_impl.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/ipc/service/... media/gpu/ipc/service/gpu_video_decode_accelerator_factory_impl.cc:72: // Query VDAs for their capabilities and construct a set of supported I think this should also stay at the same indentation level. https://codereview.chromium.org/1882373004/diff/120001/media/gpu/ipc/service/... File media/gpu/ipc/service/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/ipc/service/... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:80: "input_format=" << "input_format=" << input_format ? https://codereview.chromium.org/1882373004/diff/120001/media/gpu/ipc/service/... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:271: "frame_id=" << "frame_id=" << params.frame_id; ? This also happens in more places in this file. https://codereview.chromium.org/1882373004/diff/120001/media/gpu/ipc/service/... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:311: frame->AddDestructionObserver(media::BindToCurrentLoop(base::Bind( To be honest, I'd prefer keeping original formatting of most changes in this file. This is much less readable... https://codereview.chromium.org/1882373004/diff/120001/media/gpu/v4l2_slice_v... File media/gpu/v4l2_slice_video_decode_accelerator.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:1936: arraysize(pps->scaling_list4x4) && Is this correct formatting? https://codereview.chromium.org/1882373004/diff/120001/media/gpu/vaapi_jpeg_d... File media/gpu/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/vaapi_jpeg_d... media/gpu/vaapi_jpeg_decode_accelerator.cc:29: VAJDA_DECODER_FAILURES_MAX, Could we please keep the comment to explain why +1? https://codereview.chromium.org/1882373004/diff/120001/media/gpu/vaapi_video_... File media/gpu/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/vaapi_video_... media/gpu/vaapi_video_decode_accelerator.cc:490: // else fallthrough Please keep the original indent. https://codereview.chromium.org/1882373004/diff/120001/media/gpu/vaapi_video_... File media/gpu/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/vaapi_video_... media/gpu/vaapi_video_encode_accelerator.cc:75: VAVEA_ENCODER_FAILURES_MAX, Could we please keep the comment at the new location? However, to be honest, it feels to me that +1 belonged here, since we won't need it anymore if more values are added...
lgtm
Duplicate vt_stubs for content/common and media/gpu/ipc/service on mac
rebase
rebase
Fix namespace qualification and common media/gpu stubs location (gyp)
Fix android unittest dep (gn)
Update media/gpu visibility list to provide access to test code
Switch ID3D11VideoDevice identification from IID_... to __uuidof(...)
rebase
Fix namespace qualifier on g_test_import
Rebase and drop duplicate vt_stubs generation
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
Fix content/test/BUILD.gn rebase; add //media/gpu to content/renderer deps; sync media/gpu-related gyp and gn deps in content/renderer
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/230001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Squash and rebase
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/240001
markdittmer@chromium.org changed reviewers: + jam@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Presubmit trybot is failing on missing OWNERS for new DEPS on:
'+third_party/angle',
'+third_party/libva',
'+third_party/v4l-utils',
jam@ PTAL, just at new DEPS.
lgtm
https://codereview.chromium.org/1882373004/diff/240001/content/DEPS File content/DEPS (right): https://codereview.chromium.org/1882373004/diff/240001/content/DEPS#newcode30 content/DEPS:30: "+media", btw there are many +media lines in DEPS files under content, these can be removed now (fine in a followup)
Rebase and refresh from relevant content/common media-related changes
Fix several bot-identified issues
Fix up some broken includes
Add //media/gpu as dep for //media/gpu:*_unittest
Add missing gfx_ipc_geometry dep to media/gpu/ipc/common
Add missing dep on media_gpu to content/gpu GN build
Add missing deps and export AVDASurfaceTracker
Fix media/gpu visibility
git cl format media
Address comments from posciak@
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Revert git-cl-format-breaking changes
https://codereview.chromium.org/1882373004/diff/120001/media/gpu/h264_decoder.cc File media/gpu/h264_decoder.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/h264_decoder... media/gpu/h264_decoder.cc:1315: // else fallthrough On 2016/04/19 09:22:55, Pawel Osciak wrote: > I believe the indent here was correct. Done. https://codereview.chromium.org/1882373004/diff/120001/media/gpu/ipc/service/... File media/gpu/ipc/service/gpu_video_decode_accelerator_factory_impl.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/ipc/service/... media/gpu/ipc/service/gpu_video_decode_accelerator_factory_impl.cc:72: // Query VDAs for their capabilities and construct a set of supported On 2016/04/19 09:22:56, Pawel Osciak wrote: > I think this should also stay at the same indentation level. Done. https://codereview.chromium.org/1882373004/diff/120001/media/gpu/ipc/service/... File media/gpu/ipc/service/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/ipc/service/... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:80: "input_format=" On 2016/04/19 09:22:55, Pawel Osciak wrote: > << "input_format=" << input_format > ? Done. https://codereview.chromium.org/1882373004/diff/120001/media/gpu/ipc/service/... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:271: "frame_id=" On 2016/04/19 09:22:55, Pawel Osciak wrote: > << "frame_id=" << params.frame_id; > ? > > This also happens in more places in this file. Done. https://codereview.chromium.org/1882373004/diff/120001/media/gpu/ipc/service/... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:311: frame->AddDestructionObserver(media::BindToCurrentLoop(base::Bind( On 2016/04/19 09:22:55, Pawel Osciak wrote: > To be honest, I'd prefer keeping original formatting of most changes in this > file. This is much less readable... Done. https://codereview.chromium.org/1882373004/diff/120001/media/gpu/v4l2_slice_v... File media/gpu/v4l2_slice_video_decode_accelerator.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/v4l2_slice_v... media/gpu/v4l2_slice_video_decode_accelerator.cc:1936: arraysize(pps->scaling_list4x4) && On 2016/04/19 09:22:55, Pawel Osciak wrote: > Is this correct formatting? Done. https://codereview.chromium.org/1882373004/diff/120001/media/gpu/vaapi_jpeg_d... File media/gpu/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/vaapi_jpeg_d... media/gpu/vaapi_jpeg_decode_accelerator.cc:29: VAJDA_DECODER_FAILURES_MAX, On 2016/04/19 09:22:55, Pawel Osciak wrote: > Could we please keep the comment to explain why +1? The max is no longer greater than 1. If left, the comment would be inaccurate. https://codereview.chromium.org/1882373004/diff/120001/media/gpu/vaapi_video_... File media/gpu/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/vaapi_video_... media/gpu/vaapi_video_decode_accelerator.cc:490: // else fallthrough On 2016/04/19 09:22:55, Pawel Osciak wrote: > Please keep the original indent. Done. https://codereview.chromium.org/1882373004/diff/120001/media/gpu/vaapi_video_... File media/gpu/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/1882373004/diff/120001/media/gpu/vaapi_video_... media/gpu/vaapi_video_encode_accelerator.cc:75: VAVEA_ENCODER_FAILURES_MAX, On 2016/04/19 09:22:55, Pawel Osciak wrote: > Could we please keep the comment at the new location? However, to be honest, it > feels to me that +1 belonged here, since we won't need it anymore if more values > are added... Ditto to other comment. UMA requirements appear to have either changed or originally been misinterpreted when =2 was used. The comment no longer applies.
The CQ bit was checked by markdittmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/350001
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 markdittmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org, xhwang@chromium.org, sadrul@chromium.org, piman@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1882373004/#ps350001 (title: "Revert git-cl-format-breaking changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/350001
Message was sent while issue was closed.
Description was changed from ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 ========== to ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 ==========
Message was sent while issue was closed.
Committed patchset #37 (id:350001)
Message was sent while issue was closed.
Description was changed from ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 ========== to ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} ==========
Message was sent while issue was closed.
Patchset 37 (id:??) landed as https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896}
Message was sent while issue was closed.
Description was changed from ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} ========== to ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} ==========
Add X11 headers to vaapi_tfp_picture.cc
The CQ bit was checked by markdittmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, jam@chromium.org, agrieve@chromium.org, piman@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/1882373004/#ps360001 (title: "Add X11 headers to vaapi_tfp_picture.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Add X11 headers to vaapi_tfp_picture.cc
The CQ bit was checked by markdittmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, jam@chromium.org, agrieve@chromium.org, piman@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/1882373004/#ps370001 (title: "Add X11 headers to vaapi_tfp_picture.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/370001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/370001
Message was sent while issue was closed.
Description was changed from ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} ========== to ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} ==========
Message was sent while issue was closed.
Committed patchset #39 (id:370001)
Message was sent while issue was closed.
Description was changed from ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} ========== to ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} Committed: https://crrev.com/0f54920173bda1d13e9c89c51a1995a8542c5648 Cr-Commit-Position: refs/heads/master@{#390897} ==========
Message was sent while issue was closed.
Patchset 39 (id:??) landed as https://crrev.com/0f54920173bda1d13e9c89c51a1995a8542c5648 Cr-Commit-Position: refs/heads/master@{#390897}
Message was sent while issue was closed.
Add x11 config to x11 ChromeOS build
Message was sent while issue was closed.
Description was changed from ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} Committed: https://crrev.com/0f54920173bda1d13e9c89c51a1995a8542c5648 Cr-Commit-Position: refs/heads/master@{#390897} ========== to ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} Committed: https://crrev.com/0f54920173bda1d13e9c89c51a1995a8542c5648 Cr-Commit-Position: refs/heads/master@{#390897} ==========
rebase
The CQ bit was checked by markdittmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, jam@chromium.org, agrieve@chromium.org, piman@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/1882373004/#ps410001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882373004/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882373004/410001
Message was sent while issue was closed.
Description was changed from ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} Committed: https://crrev.com/0f54920173bda1d13e9c89c51a1995a8542c5648 Cr-Commit-Position: refs/heads/master@{#390897} ========== to ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} Committed: https://crrev.com/0f54920173bda1d13e9c89c51a1995a8542c5648 Cr-Commit-Position: refs/heads/master@{#390897} ==========
Message was sent while issue was closed.
Committed patchset #41 (id:410001)
Message was sent while issue was closed.
Description was changed from ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} Committed: https://crrev.com/0f54920173bda1d13e9c89c51a1995a8542c5648 Cr-Commit-Position: refs/heads/master@{#390897} ========== to ========== Migrate content/common/gpu/media code to media/gpu This is part of a gpu & media refactor to enable proper layering for Mus. See bug for details. BUG=586386 Committed: https://crrev.com/6e70beb84fe0d0a3d27e4369c8b6aaae001afb81 Cr-Commit-Position: refs/heads/master@{#390896} Committed: https://crrev.com/0f54920173bda1d13e9c89c51a1995a8542c5648 Cr-Commit-Position: refs/heads/master@{#390897} Committed: https://crrev.com/9204aefa88c2a7af21e288dd3f1a5b8db468002a Cr-Commit-Position: refs/heads/master@{#390908} ==========
Message was sent while issue was closed.
Patchset 41 (id:??) landed as https://crrev.com/9204aefa88c2a7af21e288dd3f1a5b8db468002a Cr-Commit-Position: refs/heads/master@{#390908}
Message was sent while issue was closed.
A revert of this CL (patchset #41 id:410001) has been created in https://codereview.chromium.org/1939083002/ by thakis@chromium.org. The reason for reverting is: Probably caused https://crbug.com/608348.
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
Also, I'm a bit surprised that this has three "landed as" lines -- how did that happen? (Makes me wonder if the revert will work correctly, too) |
