Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12998003:
Fix VideoLayerImpl upload of YUV->RGBA converted frames. (Closed)
DescriptionFix VideoLayerImpl upload of YUV->RGBA converted frames.
Fixes issues introduced by r189506.
BUG=chromium:167417
BUG=chromium-os:38376
TEST=local build, unittests
Change-Id: Ic1ed42e897a7c89b574a653ca7a7ef3fa5395375
Patch Set 1 #
Total comments: 7
Patch Set 2 : Style nits. #Patch Set 3 : Rebase. #
Messages
Total messages: 16 (0 generated)
I had some unittest failures, which this should fix. I went ahead and removed NumPlanes() in this CL as well. danakj@: PTAL
https://chromiumcodereview.appspot.com/12998003/diff/1/cc/layers/video_layer_... File cc/layers/video_layer_impl.cc (right): https://chromiumcodereview.appspot.com/12998003/diff/1/cc/layers/video_layer_... cc/layers/video_layer_impl.cc:378: case media::VideoFrame::HOLE: i don't see what prevents this from being called with HOLE? https://chromiumcodereview.appspot.com/12998003/diff/1/cc/layers/video_layer_... cc/layers/video_layer_impl.cc:456: size_t first_unused_plane = (frame_ ? media::VideoFrame::NumPlanes(format_) : how about just an early out if !frame_?
https://chromiumcodereview.appspot.com/12998003/diff/1/cc/layers/video_layer_... File cc/layers/video_layer_impl.cc (right): https://chromiumcodereview.appspot.com/12998003/diff/1/cc/layers/video_layer_... cc/layers/video_layer_impl.cc:378: case media::VideoFrame::HOLE: On 2013/03/21 22:38:31, danakj wrote: > i don't see what prevents this from being called with HOLE? NumPlanes will return 0 for HOLE, so SetupFramePlanes() won't get this far. https://chromiumcodereview.appspot.com/12998003/diff/1/cc/layers/video_layer_... cc/layers/video_layer_impl.cc:456: size_t first_unused_plane = (frame_ ? media::VideoFrame::NumPlanes(format_) : On 2013/03/21 22:38:31, danakj wrote: > how about just an early out if !frame_? We'll still want to deallocate all the planes, for example if the previous frame had planes, but the format changed for this frame to one that doesn't have planes.
LGTM https://chromiumcodereview.appspot.com/12998003/diff/1/cc/layers/video_layer_... File cc/layers/video_layer_impl.cc (right): https://chromiumcodereview.appspot.com/12998003/diff/1/cc/layers/video_layer_... cc/layers/video_layer_impl.cc:378: case media::VideoFrame::HOLE: On 2013/03/21 22:42:26, sheu wrote: > On 2013/03/21 22:38:31, danakj wrote: > > i don't see what prevents this from being called with HOLE? > > NumPlanes will return 0 for HOLE, so SetupFramePlanes() won't get this far. Ah okay. https://chromiumcodereview.appspot.com/12998003/diff/1/cc/layers/video_layer_... cc/layers/video_layer_impl.cc:456: size_t first_unused_plane = (frame_ ? media::VideoFrame::NumPlanes(format_) : On 2013/03/21 22:42:26, sheu wrote: > On 2013/03/21 22:38:31, danakj wrote: > > how about just an early out if !frame_? > > We'll still want to deallocate all the planes, for example if the previous frame > had planes, but the format changed for this frame to one that doesn't have > planes. I see! Can you wrap this a little differently, by moving the ":" to the next line just below the "?"
Updated. https://chromiumcodereview.appspot.com/12998003/diff/1/cc/layers/video_layer_... File cc/layers/video_layer_impl.cc (right): https://chromiumcodereview.appspot.com/12998003/diff/1/cc/layers/video_layer_... cc/layers/video_layer_impl.cc:456: size_t first_unused_plane = (frame_ ? media::VideoFrame::NumPlanes(format_) : On 2013/03/21 22:47:53, danakj wrote: > I see! Can you wrap this a little differently, by moving the ":" to the next > line just below the "?" That's the way I would have done things, but I wasn't sure about the style nits :-P Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/12998003/4002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/12998003/4002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/12998003/4002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/12998003/4002
Rebasing and resubmitting. I'm getting unrelated (but persistent) trybot failures, as well as trybots that just won't run. Hopefully this clears them out.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/12998003/36001
On 2013/03/22 17:36:25, sheu wrote: > Rebasing and resubmitting. I'm getting unrelated (but persistent) trybot > failures, as well as trybots that just won't run. Hopefully this clears them > out. Why not revert r189506 and resubmit it alongside the fix from this CL when the bots go green?
On 2013/03/22 20:15:05, slavi wrote: > On 2013/03/22 17:36:25, sheu wrote: > > Rebasing and resubmitting. I'm getting unrelated (but persistent) trybot > > failures, as well as trybots that just won't run. Hopefully this clears them > > out. > > Why not revert r189506 and resubmit it alongside the fix from this CL when the > bots go green? I don't have submit privileges, so I figured just submitting the change on top would be faster than submitting a revert and then a change. This is maybe not turning out to be the case? In any case I'm hearing from IRC that the CQ got messed up yesterday/last night, so I've resubmitted this change in another issue: https://codereview.chromium.org/12634034/ and I'll see how that goes.
On 2013/03/22 20:19:19, sheu wrote: > On 2013/03/22 20:15:05, slavi wrote: > > On 2013/03/22 17:36:25, sheu wrote: > > > Rebasing and resubmitting. I'm getting unrelated (but persistent) trybot > > > failures, as well as trybots that just won't run. Hopefully this clears > them > > > out. > > > > Why not revert r189506 and resubmit it alongside the fix from this CL when the > > bots go green? > > I don't have submit privileges, so I figured just submitting the change on top > would be faster than submitting a revert and then a change. This is maybe not > turning out to be the case? If the bots are red due to your patch and you do not have commit privileges, ask a sheriff to revert on #chromium.
On 2013/03/22 20:19:19, sheu wrote: > I don't have submit privileges, so I figured just submitting the change on top > would be faster than submitting a revert and then a change. This is maybe not > turning out to be the case? If you don't have commit privileges and you're reasonably certain your change is OK, just ask someone else to land manually. I've been wanting to land your change for you for a day or so but I've never heard clearly that you actually believe it's OK :) > In any case I'm hearing from IRC that the CQ got messed up yesterday/last night, > so I've resubmitted this change in another issue: > > https://codereview.chromium.org/12634034/ > > and I'll see how that goes. :/ Making a new issue for the same code change shouldn't be necessary regardless... but if you do, close the old change with the (X) in the upper-left corner of the page. I'll close this one for you.
On Fri, Mar 22, 2013 at 7:15 PM, <pkasting@chromium.org> wrote: > If you don't have commit privileges and you're reasonably certain your > change is > OK, just ask someone else to land manually. I've been wanting to land your > change for you for a day or so but I've never heard clearly that you > actually > believe it's OK :) I do think it is OK for submitting manually; if you can do that, we can get back to testing usefully on the failing bots. Thanks, -John Sheu |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
