Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
978263002:
gpu: media: initialize ozone in vaapi_h264_decoder_unittest (Closed)
|
Created:
5 years, 9 months ago by llandwerlin-old Modified:
5 years, 9 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org, ozone-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiongpu: media: initialize ozone in vaapi_h264_decoder_unittest
TEST=run vaapi_h264_decoder_unittest on ozone/freon
BUG=none
Committed: https://crrev.com/2db04a2c0999a969455cae159e5ba54f0612705c
Cr-Commit-Position: refs/heads/master@{#320709}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Allow --ozone-platform= option #
Total comments: 2
Patch Set 3 : Pawel's nit #Messages
Total messages: 15 (3 generated)
lionel.g.landwerlin@intel.com changed reviewers: + posciak@chromium.org, tiago.vignatti@intel.com
Pawel, my colleague Tiago just pointed out this unit test is broken on Ozone.
with the changes I mentioned: LGTM. https://codereview.chromium.org/978263002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/978263002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:371: continue; llandwerlin@, this CL works for me if I amend the following snip and start the test using "--ozone-platform=gbm": + if (it->first == "ozone-platform" || it->first == "ozone-use-surfaceless") + continue;
https://codereview.chromium.org/978263002/diff/1/content/common/gpu/media/vaa... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/978263002/diff/1/content/common/gpu/media/vaa... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:371: continue; On 2015/03/05 17:56:41, vignatti wrote: > llandwerlin@, this CL works for me if I amend the following snip and start the > test using "--ozone-platform=gbm": > > + if (it->first == "ozone-platform" || it->first == "ozone-use-surfaceless") > + continue; Do we really need the surfaceless switch? This test doesn't seem to render anything.
On 2015/03/06 06:48:49, llandwerlin wrote: > https://codereview.chromium.org/978263002/diff/1/content/common/gpu/media/vaa... > File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): > > https://codereview.chromium.org/978263002/diff/1/content/common/gpu/media/vaa... > content/common/gpu/media/vaapi_h264_decoder_unittest.cc:371: continue; > On 2015/03/05 17:56:41, vignatti wrote: > > llandwerlin@, this CL works for me if I amend the following snip and start the > > test using "--ozone-platform=gbm": > > > > + if (it->first == "ozone-platform" || it->first == > "ozone-use-surfaceless") > > + continue; > > Do we really need the surfaceless switch? This test doesn't seem to render > anything. you're right about that. It was a bad copy & pasta :)
On 2015/03/06 15:45:40, vignatti wrote: > On 2015/03/06 06:48:49, llandwerlin wrote: > > > https://codereview.chromium.org/978263002/diff/1/content/common/gpu/media/vaa... > > File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): > > > > > https://codereview.chromium.org/978263002/diff/1/content/common/gpu/media/vaa... > > content/common/gpu/media/vaapi_h264_decoder_unittest.cc:371: continue; > > On 2015/03/05 17:56:41, vignatti wrote: > > > llandwerlin@, this CL works for me if I amend the following snip and start > the > > > test using "--ozone-platform=gbm": > > > > > > + if (it->first == "ozone-platform" || it->first == > > "ozone-use-surfaceless") > > > + continue; > > > > Do we really need the surfaceless switch? This test doesn't seem to render > > anything. > > you're right about that. It was a bad copy & pasta :) llandwerlin@ do you want upload maybe a second patch set with the changed I mentioned? posciak@ wdyt overall?
On 2015/03/11 00:29:22, vignatti wrote: > On 2015/03/06 15:45:40, vignatti wrote: > > On 2015/03/06 06:48:49, llandwerlin wrote: > > > > > > https://codereview.chromium.org/978263002/diff/1/content/common/gpu/media/vaa... > > > File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/978263002/diff/1/content/common/gpu/media/vaa... > > > content/common/gpu/media/vaapi_h264_decoder_unittest.cc:371: continue; > > > On 2015/03/05 17:56:41, vignatti wrote: > > > > llandwerlin@, this CL works for me if I amend the following snip and start > > the > > > > test using "--ozone-platform=gbm": > > > > > > > > + if (it->first == "ozone-platform" || it->first == > > > "ozone-use-surfaceless") > > > > + continue; > > > > > > Do we really need the surfaceless switch? This test doesn't seem to render > > > anything. > > > > you're right about that. It was a bad copy & pasta :) > > llandwerlin@ do you want upload maybe a second patch set with the changed I > mentioned? > > posciak@ wdyt overall? I agree.
PTAL, Thanks.
lgtm % nit https://codereview.chromium.org/978263002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/978263002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:19: #include "third_party/libyuv/include/libyuv.h" Please add an empty line here.
https://codereview.chromium.org/978263002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/978263002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:19: #include "third_party/libyuv/include/libyuv.h" On 2015/03/16 04:37:59, Pawel Osciak wrote: > Please add an empty line here. Done.
The CQ bit was checked by lionel.g.landwerlin@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from tiago.vignatti@intel.com, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/978263002/#ps40001 (title: "Pawel's nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/978263002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2db04a2c0999a969455cae159e5ba54f0612705c Cr-Commit-Position: refs/heads/master@{#320709} |
