Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11413005:
YUV software decode path stride fixes. (Closed)
|
Created:
8 years, 1 month ago by sheu Modified:
8 years, 1 month ago Reviewers:
jamesr, reveman, jar (doing other things), Ami GONE FROM CHROMIUM, Alpha Left Google, enne (OOO), danakj, ddorwin CC:
chromium-reviews, feature-media-reviews_chromium.org, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionYUV software decode path stride fixes.
Fix YUV software-decoded video playback for frame with padded strides.
* Use per-plane stride info from VideoFrame for source image rect width, when
uploading a CPU-allocated video frame.
* Modify cc::TextureUploader to handle non-4-byte-aligned texture uploads.
* Add testcase UploadContentsTest to check texture upload paths.
* Add RoundDown()/RoundUp() to base::bits and use them in media::VideoFrame
and cc::TextureUploader.
BUG=chromium:160622, chromium:161023
TEST=local build, run on x86
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168970
Patch Set 1 #
Total comments: 15
Patch Set 2 : #Patch Set 3 : <WIP> #
Total comments: 7
Patch Set 4 : Fix without touching VideoFrame #Patch Set 5 : #Patch Set 6 : Unittest segfault fix.wq #
Total comments: 3
Patch Set 7 : Added alignment comment. #
Total comments: 2
Patch Set 8 : RoundUp() in base::bits #Patch Set 9 : With unittests. #
Total comments: 1
Patch Set 10 : Rebse. #Patch Set 11 : Removed set of GL_UNPACK_ALIGNMENT #
Total comments: 15
Patch Set 12 : WebKit nits. #
Total comments: 1
Patch Set 13 : cc LGTM'ed. #
Total comments: 2
Patch Set 14 : More robust RoundUp #Patch Set 15 : Without base bits, <grumble> #Patch Set 16 : Nits. #
Total comments: 1
Patch Set 17 : More nits. #
Total comments: 2
Messages
Total messages: 51 (0 generated)
PTAL.
So confused... https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc File cc/video_layer_impl.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc... cc/video_layer_impl.cc:209: float texHeightScale = Undo these changes? https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc... cc/video_layer_impl.cc:219: // padding past what coded_size_ reflects. I don't see how this CL changes things; how is yPlane.size not exactly equal to codedSize? https://chromiumcodereview.appspot.com/11413005/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/1/media/base/video_frame... media/base/video_frame.cc:158: size_t uv_stride = y_stride / 2; I don't understand the impetus for this change. Why is the old code wrong / why is it important that y_stride==uv_stride*2?
https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc File cc/video_layer_impl.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc... cc/video_layer_impl.cc:209: float texHeightScale = On 2012/11/15 18:15:36, Ami Fischman wrote: > Undo these changes? They're applicable for the rest of the paths, so I think it would be good to calculate them once here. The YUV software decoder path is an exception. https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc... cc/video_layer_impl.cc:219: // padding past what coded_size_ reflects. On 2012/11/15 18:15:36, Ami Fischman wrote: > I don't see how this CL changes things; how is yPlane.size not exactly equal to > codedSize? See video_frame.cc, where we do some extra alignment to 16 bytes for software-allocated YUV frames. It would be better to get coded_size to match up, but that's more changes that would complicate the immediate fix. https://chromiumcodereview.appspot.com/11413005/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/1/media/base/video_frame... media/base/video_frame.cc:158: size_t uv_stride = y_stride / 2; On 2012/11/15 18:15:36, Ami Fischman wrote: > I don't understand the impetus for this change. > Why is the old code wrong / why is it important that y_stride==uv_stride*2? When we're doing GL rendering of the uploaded texture, we have only one scale factor for the Y and UV planes. If the strides/height aspect ratios aren't the same, we'll get chroma and luma misaligned with each other. I'll add this to the comment.
Happy to VC if that'll help... https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc File cc/video_layer_impl.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc... cc/video_layer_impl.cc:209: float texHeightScale = On 2012/11/15 19:46:38, sheu wrote: > On 2012/11/15 18:15:36, Ami Fischman wrote: > > Undo these changes? > > They're applicable for the rest of the paths, so I think it would be good to > calculate them once here. The YUV software decoder path is an exception. My point was that all you changed was dropping the "const" but then you never reassign. So AFAICT dropping the const is not useful. https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc... cc/video_layer_impl.cc:219: // padding past what coded_size_ reflects. On 2012/11/15 19:46:38, sheu wrote: > On 2012/11/15 18:15:36, Ami Fischman wrote: > > I don't see how this CL changes things; how is yPlane.size not exactly equal > to > > codedSize? > > See video_frame.cc, where we do some extra alignment to 16 bytes for > software-allocated YUV frames. I somehow missed the thunk below in videoFrameDimension. Oops. https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc... cc/video_layer_impl.cc:221: static_cast<float>(visibleRect.width()) / yPlane.size.width(), What is this supposed to be scaling from/to? My assumption _was_ that this is meant to scale from a VideoFrame's visible_size() to a <video>'s height/width. But AFAICT nothing in this class knows about the web-visible dimensions, instead everything is keyed off the VideoFrame's idea of its own size(s). I must be misunderstanding something, because ISTM that (say) if you had a visible width of 20 pixels and a stride of 200 pixels (because why not), this would end up with a scaling factor of 0.1; something's gonna be multiplied or divided by 10. What is that something? https://chromiumcodereview.appspot.com/11413005/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/1/media/base/video_frame... media/base/video_frame.cc:158: size_t uv_stride = y_stride / 2; On 2012/11/15 19:46:38, sheu wrote: > On 2012/11/15 18:15:36, Ami Fischman wrote: > > I don't understand the impetus for this change. > > Why is the old code wrong / why is it important that y_stride==uv_stride*2? > > When we're doing GL rendering of the uploaded texture, we have only one scale > factor for the Y and UV planes. If the strides/height aspect ratios aren't the > same, we'll get chroma and luma misaligned with each other. It seems strange that an impl detail of cc::VideoLayerImpl should place this requirement on VideoFrame. Shouldn't this instead imply that the shader should take multiple scale factors, one per plane?
https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc File cc/video_layer_impl.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc... cc/video_layer_impl.cc:209: float texHeightScale = On 2012/11/15 20:39:09, Ami Fischman wrote: > On 2012/11/15 19:46:38, sheu wrote: > > On 2012/11/15 18:15:36, Ami Fischman wrote: > > > Undo these changes? > > > > They're applicable for the rest of the paths, so I think it would be good to > > calculate them once here. The YUV software decoder path is an exception. > > My point was that all you changed was dropping the "const" but then you never > reassign. So AFAICT dropping the const is not useful. Ah right, no-op change. Fixed. https://chromiumcodereview.appspot.com/11413005/diff/1/cc/video_layer_impl.cc... cc/video_layer_impl.cc:221: static_cast<float>(visibleRect.width()) / yPlane.size.width(), On 2012/11/15 20:39:09, Ami Fischman wrote: > What is this supposed to be scaling from/to? > > My assumption _was_ that this is meant to scale from a VideoFrame's > visible_size() to a <video>'s height/width. But AFAICT nothing in this class > knows about the web-visible dimensions, instead everything is keyed off the > VideoFrame's idea of its own size(s). > I must be misunderstanding something, because ISTM that (say) if you had a > visible width of 20 pixels and a stride of 200 pixels (because why not), this > would end up with a scaling factor of 0.1; something's gonna be multiplied or > divided by 10. What is that something? We're not scaling to <video> dimensions -- that's handled by quadRect, which is constructed from contentBounds(). We're scaling the video texture that we're sampling from, since the borders of the video texture might have extraneous data that we don't use. So with a stride of 200 pixels, and a visible width of 20, we're scaling with a factor of 0.1, regardless of the <video> size; the size of the <video> will just affect how big that 10% of the video texture we do use is displayed in the final composited output. https://chromiumcodereview.appspot.com/11413005/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/1/media/base/video_frame... media/base/video_frame.cc:158: size_t uv_stride = y_stride / 2; On 2012/11/15 20:39:09, Ami Fischman wrote: > On 2012/11/15 19:46:38, sheu wrote: > > On 2012/11/15 18:15:36, Ami Fischman wrote: > > > I don't understand the impetus for this change. > > > Why is the old code wrong / why is it important that y_stride==uv_stride*2? > > > > When we're doing GL rendering of the uploaded texture, we have only one scale > > factor for the Y and UV planes. If the strides/height aspect ratios aren't > the > > same, we'll get chroma and luma misaligned with each other. > > It seems strange that an impl detail of cc::VideoLayerImpl should place this > requirement on VideoFrame. > > Shouldn't this instead imply that the shader should take multiple scale factors, > one per plane? I guess that would have been a better way to put this -- we're implicitly assuming that scale factors are identical for Y and UV. It's not necessarily just aa cc::VideoLayerImpl implementation detaiil.
Updated.
https://chromiumcodereview.appspot.com/11413005/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/1/media/base/video_frame... media/base/video_frame.cc:158: size_t uv_stride = y_stride / 2; On 2012/11/15 22:20:53, sheu wrote: > On 2012/11/15 20:39:09, Ami Fischman wrote: > > On 2012/11/15 19:46:38, sheu wrote: > > > On 2012/11/15 18:15:36, Ami Fischman wrote: > > > > I don't understand the impetus for this change. > > > > Why is the old code wrong / why is it important that > y_stride==uv_stride*2? > > > > > > When we're doing GL rendering of the uploaded texture, we have only one > scale > > > factor for the Y and UV planes. If the strides/height aspect ratios aren't > > the > > > same, we'll get chroma and luma misaligned with each other. > > > > It seems strange that an impl detail of cc::VideoLayerImpl should place this > > requirement on VideoFrame. > > > > Shouldn't this instead imply that the shader should take multiple scale > factors, > > one per plane? > > I guess that would have been a better way to put this -- we're implicitly > assuming that scale factors are identical for Y and UV. That seems wrong. stride can differ per-plane, and width cannot. Therefore their ratio can differ per plane. > It's not necessarily just aa cc::VideoLayerImpl implementation detaiil. I don't follow. Who else cares? Can you assume the presence of GL_EXT_unpack_subimage and use GL_UNPACK_ROW_LENGTH to tell GL exactly what's going on (stride!=width) instead of scaling to have the visible rect cover the stride and then get scaled back down to the <video> size?
https://chromiumcodereview.appspot.com/11413005/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/1/media/base/video_frame... media/base/video_frame.cc:158: size_t uv_stride = y_stride / 2; On 2012/11/15 23:11:15, Ami Fischman wrote: > That seems wrong. stride can differ per-plane, and width cannot. Therefore > their ratio can differ per plane. Right, and I'm making sure that the planes have the same ratio here. On 2012/11/15 23:11:15, Ami Fischman wrote: > I don't follow. Who else cares? > > > Can you assume the presence of GL_EXT_unpack_subimage and use > GL_UNPACK_ROW_LENGTH to tell GL exactly what's going on (stride!=width) instead > of scaling to have the visible rect cover the stride and then get scaled back > down to the <video> size? I'm not using the visible rect to cover both the stride and the <video> size scaling. Basically: * <video> size scaling is handled by the quadRect, which affects the vertex coordinates when we draw the textured quad to the screen. * visible/coded/stride scaling is handled by the texScale factor, which affects texture coordinates when we draw the textured quad. I've uploaded a next WIP version to get your thoughts on it. GL_EXT_unpack_subimage has some other problems that I'll notate on that version.
https://chromiumcodereview.appspot.com/11413005/diff/9001/media/base/video_fr... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/9001/media/base/video_fr... media/base/video_frame.cc:127: coded_size_.set_width(RoundUp(coded_size_.width(), 4)); This is kinda hacky. I'm doing this basically because I don't want to have to touch glStorePixeli(GL_UNPACK_ALIGNMENT), as if the coded size can be arbitrarily aligned, then we have to set the unpack alignment to 1 when we do the texture upload. GL_EXT_unpack_subimage still gets bit by the unpack alignment.
This should be better. PTAL.
https://chromiumcodereview.appspot.com/11413005/diff/9001/cc/video_layer_impl.cc File cc/video_layer_impl.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/9001/cc/video_layer_impl... cc/video_layer_impl.cc:290: gfx::Size dimensions = gfx::Size(frame->coded_size()); lolwat? https://chromiumcodereview.appspot.com/11413005/diff/9001/cc/video_layer_impl... cc/video_layer_impl.cc:375: gfx::Rect imageRect(0, 0, m_frame->stride(planeIndex), plane.size.height()); since this is going to copy anyway why not crop the stride/padding at this point?
https://chromiumcodereview.appspot.com/11413005/diff/9001/cc/video_layer_impl.cc File cc/video_layer_impl.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/9001/cc/video_layer_impl... cc/video_layer_impl.cc:290: gfx::Size dimensions = gfx::Size(frame->coded_size()); On 2012/11/16 22:15:43, Ami Fischman wrote: > lolwat? You're looking at a CL 2 revisions old :-) https://chromiumcodereview.appspot.com/11413005/diff/9001/cc/video_layer_impl... cc/video_layer_impl.cc:375: gfx::Rect imageRect(0, 0, m_frame->stride(planeIndex), plane.size.height()); On 2012/11/16 22:15:43, Ami Fischman wrote: > since this is going to copy anyway why not crop the stride/padding at this > point? Using m_frame->stride() here is just slightly hacky, since the frame data size should be exactly coded_size. So when I do the upload, I make the size coded_size to "true things up" again. I suppose I could upload to visible_rect, and not have to do scaling on render, but then the texture size no longer reflects coded_size, and I'd like to avoid that.
https://chromiumcodereview.appspot.com/11413005/diff/9001/cc/video_layer_impl.cc File cc/video_layer_impl.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/9001/cc/video_layer_impl... cc/video_layer_impl.cc:375: gfx::Rect imageRect(0, 0, m_frame->stride(planeIndex), plane.size.height()); On 2012/11/17 00:35:39, sheu wrote: > On 2012/11/16 22:15:43, Ami Fischman wrote: > > since this is going to copy anyway why not crop the stride/padding at this > > point? > > Using m_frame->stride() here is just slightly hacky, since the frame data size > should be exactly coded_size. So when I do the upload, I make the size > coded_size to "true things up" again. > > I suppose I could upload to visible_rect, and not have to do scaling on render, > but then the texture size no longer reflects coded_size, and I'd like to avoid > that. Why? ISTM the cleanest thing would be to make the texture reflect visible_rect; and make *this* the only place that needs to think about VideoFrame::stride(). Having more than one place in cc:: know about media::'s notions of stride seems like a recipe for later confusion (and possibly missing cases now).
https://chromiumcodereview.appspot.com/11413005/diff/9001/cc/video_layer_impl.cc File cc/video_layer_impl.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/9001/cc/video_layer_impl... cc/video_layer_impl.cc:375: gfx::Rect imageRect(0, 0, m_frame->stride(planeIndex), plane.size.height()); On 2012/11/17 00:41:37, Ami Fischman wrote: > On 2012/11/17 00:35:39, sheu wrote: > > On 2012/11/16 22:15:43, Ami Fischman wrote: > > > since this is going to copy anyway why not crop the stride/padding at this > > > point? > > > > Using m_frame->stride() here is just slightly hacky, since the frame data size > > should be exactly coded_size. So when I do the upload, I make the size > > coded_size to "true things up" again. > > > > I suppose I could upload to visible_rect, and not have to do scaling on > render, > > but then the texture size no longer reflects coded_size, and I'd like to avoid > > that. > > Why? ISTM the cleanest thing would be to make the texture reflect visible_rect; > and make *this* the only place that needs to think about VideoFrame::stride(). > Having more than one place in cc:: know about media::'s notions of stride seems > like a recipe for later confusion (and possibly missing cases now). This is the only place that knows about stride now. If you diff against the base now, it's also the only three lines changed in the file. So, with this change: coded_size refers to the frame data size, just like everywhere else in the code. visible_rect refers to the visible rect, just like everywhere else in the code. If I pre-cropped, then visible_rect would be incorrect in the case that there was an x,y offset, and coded_size would refer to a size larger than the actual frame data size.
> If I pre-cropped, then visible_rect would be incorrect in the case that there > was an x,y offset Why? > and coded_size would refer to a size larger than the actual frame data size. I think this is the crux of the matter - coded_size is a VideoFrame concept; once you pass the VideoLayer interface you're dealing with textures and other cc concepts. Is it the case that anything beyond cc::VLI (on the cc side) inspects VFs? (if yes, then I agree staying "compatible" is a plus, but also probably a dep that should be broken). Anyway, this is no longer touching media/ so you should take my remaining comments as drive-by only; get someone who actually knows how the compositor works to review this :) (also, I'll be on vacation the next week so don't wait for me to land)
@jamesr should look at video layer. the layer change itself seems fine to me, but Ami seems concerned about layering violations. @reveman should look at texture uploader. i think that code is a little opaque, and its intent could be a lot more clear by going about it a little differently, and/or using intermediate variables with descriptive names instead of constants. https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader.cc File cc/texture_uploader.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader... cc/texture_uploader.cc:233: (bytes_per_pixel * source_rect.width() + 3) & ~0x3; I'm certain this can be written in a way that makes its intentions more clear. https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader... cc/texture_uploader.cc:302: (bytes_per_pixel * source_rect.width() + 3) & ~0x3; same. helper method?
I'm not sure why you need to patch the uploader, but if you need some change in behavior then you need unit tests to cover that change and your desired behavior. https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader.cc File cc/texture_uploader.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader... cc/texture_uploader.cc:232: unsigned int upload_image_stride = why is this change necessary?
On 2012/11/19 04:14:46, jamesr wrote: > I'm not sure why you need to patch the uploader, but if you need some change in > behavior then you need unit tests to cover that change and your desired > behavior. > > https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader.cc > File cc/texture_uploader.cc (right): > > https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader... > cc/texture_uploader.cc:232: unsigned int upload_image_stride = > why is this change necessary? @sheu, do you expect to address these concerns very soon or should we revert r166988? This bug is affecting other development efforts.
On 2012/11/17 07:51:56, Ami Fischman wrote: > > If I pre-cropped, then visible_rect would be incorrect in the case that there > > was an x,y offset > > Why? Say the visible_rect is (1, 1, 6, 4). Once visible_rect is applied, then the frame data only covers a 6x4 area, but now with the origin at (0, 0), and the visible_rect now would indicate a 6x4 area with the origin at (1, 1) for the new frame data. This is incorrect since (1) the new visible_rect should have an origin at (0, 0), and (2) the visible_rect is now also larger than the frame data, since it's offset from the origin. > > and coded_size would refer to a size larger than the actual frame data size. > > I think this is the crux of the matter - coded_size is a VideoFrame concept; > once you pass the VideoLayer interface you're dealing with textures and other cc > concepts. Is it the case that anything beyond cc::VLI (on the cc side) inspects > VFs? (if yes, then I agree staying "compatible" is a plus, but also probably a > dep that should be broken). I don't see that anything else touches VideoFrame, but VLI itself assumes on all other paths that coded_size and visible_rect are consistent with each other, and I would like that to stay the case.
On 2012/11/17 19:25:14, danakj wrote: > @jamesr should look at video layer. the layer change itself seems fine to me, > but Ami seems concerned about layering violations. As far as layering violations go, unless we forced VideoFrame itself to match stride to coded_size for all video planes (which fischman@ dissuaded me from in comment #7), we're going to have to do this violation *somewhere* when we access VideoFrame, and this is the least intrusive way to do it. > @reveman should look at texture uploader. i think that code is a little opaque, > and its intent could be a lot more clear by going about it a little differently, > and/or using intermediate variables with descriptive names instead of constants. Added a comment, to indicate the 4-byte alignment. I think the math is idiomatic enough for alignment that it's not worth breaking out into another variable. > cc/texture_uploader.cc:302: (bytes_per_pixel * source_rect.width() + 3) & ~0x3; > same. helper method? If there's a "round size up to nearest multiple" macro already in Chrome somewhere (similar to VideoFrame::RoundUp"), I'm not really inclined to add one for just this case. Or should I?
On 2012/11/19 04:14:46, jamesr wrote: > I'm not sure why you need to patch the uploader, but if you need some change in > behavior then you need unit tests to cover that change and your desired > behavior. The existing unittest doesn't check the texture contents after upload; it just checks that uploading succeeds. It might not even be possible to check the texture contents, since we're using FakeWebGraphicsContext3D for these tests, which just no-ops the actual operation. Since the only way to check if the upload stride is correct is to check the upload product for corruption, I'm not sure how to go about unittesting this (unless I suddenly make these unittests much more heavyweight by requiring an actual live OpenGL context). > https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader.cc > File cc/texture_uploader.cc (right): > > https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader... > cc/texture_uploader.cc:232: unsigned int upload_image_stride = > why is this change necessary? The point of these changes it that the current uploader does not handle uploading row sizes that aren't a multiple of 4 bytes, causing corruption in that case.
On 2012/11/19 22:12:47, ddorwin wrote: > On 2012/11/19 04:14:46, jamesr wrote: > > I'm not sure why you need to patch the uploader, but if you need some change > in > > behavior then you need unit tests to cover that change and your desired > > behavior. > > > > > https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader.cc > > File cc/texture_uploader.cc (right): > > > > > https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader... > > cc/texture_uploader.cc:232: unsigned int upload_image_stride = > > why is this change necessary? > > @sheu, do you expect to address these concerns very soon or should we revert > r166988? This bug is affecting other development efforts. Working on it. Bug the reviewers :-)
https://chromiumcodereview.appspot.com/11413005/diff/10007/cc/texture_uploade... File cc/texture_uploader.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/10007/cc/texture_uploade... cc/texture_uploader.cc:234: (bytes_per_pixel * source_rect.width() + 3) & ~0x3; Oh, I see. This is ((image_stride + 3) / 4) * 4. Can you just write it like that?
https://chromiumcodereview.appspot.com/11413005/diff/10007/cc/texture_uploade... File cc/texture_uploader.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/10007/cc/texture_uploade... cc/texture_uploader.cc:234: (bytes_per_pixel * source_rect.width() + 3) & ~0x3; On 2012/11/19 22:57:00, danakj wrote: > Oh, I see. This is ((image_stride + 3) / 4) * 4. Can you just write it like > that? Or, make a nice descriptively named static function to do this. That'd be quite nice.
On 2012/11/19 22:58:11, danakj wrote: > https://chromiumcodereview.appspot.com/11413005/diff/10007/cc/texture_uploade... > File cc/texture_uploader.cc (right): > > https://chromiumcodereview.appspot.com/11413005/diff/10007/cc/texture_uploade... > cc/texture_uploader.cc:234: (bytes_per_pixel * source_rect.width() + 3) & ~0x3; > On 2012/11/19 22:57:00, danakj wrote: > > Oh, I see. This is ((image_stride + 3) / 4) * 4. Can you just write it like > > that? > > Or, make a nice descriptively named static function to do this. That'd be quite > nice. I'd just as soon add this to a header somewhere in base/ so we don't have to re-invent this wheel every time. Do you have a preference as to where it should go?
On 2012/11/19 23:08:50, sheu wrote: > I'd just as soon add this to a header somewhere in base/ so we don't have to > re-invent this wheel every time. Do you have a preference as to where it should > go? That'd be great, I have no preference.
On 2012/11/19 22:53:18, sheu wrote: > > > https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader.cc > > File cc/texture_uploader.cc (right): > > > > > https://chromiumcodereview.appspot.com/11413005/diff/3002/cc/texture_uploader... > > cc/texture_uploader.cc:232: unsigned int upload_image_stride = > > why is this change necessary? > > The point of these changes it that the current uploader does not handle > uploading row sizes that aren't a multiple of 4 bytes, causing corruption in > that case. OK. You need tests for this, then. You can override tex*Image2D() (and pixelStorei for that matter) in a subclass of FakeWebGraphicsContext3D in your test harness and do whatever verification you like of the data. I can't tell from inspection what about the current code is buggy for rows that aren't a multiple of 4 bytes. What format is this with? My understanding is uploads for video are 1 byte/pixel so I'm surprised there is any alignment requirement.
On 2012/11/19 23:29:00, jamesr wrote: > On 2012/11/19 22:53:18, sheu wrote: > > The point of these changes it that the current uploader does not handle > > uploading row sizes that aren't a multiple of 4 bytes, causing corruption in > > that case. > > OK. You need tests for this, then. You can override tex*Image2D() (and > pixelStorei for that matter) in a subclass of FakeWebGraphicsContext3D in your > test harness and do whatever verification you like of the data. > > I can't tell from inspection what about the current code is buggy for rows that > aren't a multiple of 4 bytes. What format is this with? My understanding is > uploads for video are 1 byte/pixel so I'm surprised there is any alignment > requirement. The buggy bit is that GL_UNPACK_ALIGNMENT sets OpenGL's row stride alignment. For example, if we have it set to 4 but upload a texture with row byte stride of 6, then during OpenGL's pixel unpack, the first row would be unpacked correctly, the second being offset by 2 pixels, the third by 4, etc. So it's a matter of OpenGL state not being set correctly. If I were to write an unittest for this, I would have to essentially reimplement a chunk of the OpenGL spec, with all the stride and alignment fixins', but it seems that all the unittest is interested in really is making sure the API calls succeed and the asynchronous upload accounting works. Should I actually do this?
On 2012/11/19 23:46:29, sheu wrote: > The buggy bit is that GL_UNPACK_ALIGNMENT sets OpenGL's row stride alignment. > For example, if we have it set to 4 but upload a texture with row byte stride of > 6, then during OpenGL's pixel unpack, the first row would be unpacked correctly, > the second being offset by 2 pixels, the third by 4, etc. So it's a matter of > OpenGL state not being set correctly. OK...but without your patch we don't set GL_UNPACK_ALIGNMENT and just upload what we get. Reading between the lines, it sounds like what's going on is the data being passed from the video frame has different alignment requirements from all other data sources into texture_uploader. Is that correct? If so, what exactly are the requirements from video frames? How do you know that this change will not impact anyone else that uses this class? > > If I were to write an unittest for this, I would have to essentially reimplement > a chunk of the OpenGL spec, with all the stride and alignment fixins', but it > seems that all the unittest is interested in really is making sure the API calls > succeed and the asynchronous upload accounting works. Should I actually do > this? I don't think I fully understand the change you are trying to make, so perhaps my testing comments aren't useful yet.
On 2012/11/19 23:52:47, jamesr wrote: > On 2012/11/19 23:46:29, sheu wrote: > > The buggy bit is that GL_UNPACK_ALIGNMENT sets OpenGL's row stride alignment. > > For example, if we have it set to 4 but upload a texture with row byte stride > of > > 6, then during OpenGL's pixel unpack, the first row would be unpacked > correctly, > > the second being offset by 2 pixels, the third by 4, etc. So it's a matter of > > OpenGL state not being set correctly. > > OK...but without your patch we don't set GL_UNPACK_ALIGNMENT and just upload > what we get. Reading between the lines, it sounds like what's going on is the > data being passed from the video frame has different alignment requirements from > all other data sources into texture_uploader. Is that correct? If so, what > exactly are the requirements from video frames? How do you know that this > change will not impact anyone else that uses this class? The default GL_UNPACK_ALIGNMENT value is 4, and we don't appear to be setting it anywhere else. So the fact that we haven't seen this bug until now is just because we haven't tried uploading textures that aren't multiple-of-4-byte-strided before. This change explicitly sets the alignment to 4, and then also handles non-multiple-of-4-byte-strided uploads. Meanwhile, it doesn't affect present users because so far they've happened to be already aligned and will not be realigned. CPU-decoded video frames do happen to be non-aligned, so I fix that now.
On 2012/11/20 00:04:12, sheu wrote: > On 2012/11/19 23:52:47, jamesr wrote: > > On 2012/11/19 23:46:29, sheu wrote: > > > The buggy bit is that GL_UNPACK_ALIGNMENT sets OpenGL's row stride > alignment. > > > For example, if we have it set to 4 but upload a texture with row byte > stride > > of > > > 6, then during OpenGL's pixel unpack, the first row would be unpacked > > correctly, > > > the second being offset by 2 pixels, the third by 4, etc. So it's a matter > of > > > OpenGL state not being set correctly. > > > > OK...but without your patch we don't set GL_UNPACK_ALIGNMENT and just upload > > what we get. Reading between the lines, it sounds like what's going on is the > > data being passed from the video frame has different alignment requirements > from > > all other data sources into texture_uploader. Is that correct? If so, what > > exactly are the requirements from video frames? How do you know that this > > change will not impact anyone else that uses this class? > > The default GL_UNPACK_ALIGNMENT value is 4, and we don't appear to be setting it > anywhere else. So the fact that we haven't seen this bug until now is just > because we haven't tried uploading textures that aren't > multiple-of-4-byte-strided before. This change explicitly sets the alignment to > 4, and then also handles non-multiple-of-4-byte-strided uploads. Meanwhile, it > doesn't affect present users because so far they've happened to be already > aligned and will not be realigned. CPU-decoded video frames do happen to be > non-aligned, so I fix that now. OK. If it's already set, then don't issue another pixelStorei - the compositor should not issue redundant calls. Since this code is simply adjusting the pointer values we're making to the context, a test that verifies that the pointer offsets are calculated correctly should provide reasonable test coverage. I think you have to do that and move the copy-pasted code to a shared location before landing.
Alrighty, added a unittest for y'all unittest fiends.
https://chromiumcodereview.appspot.com/11413005/diff/6006/cc/texture_uploader.cc File cc/texture_uploader.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/6006/cc/texture_uploader... cc/texture_uploader.cc:236: m_context->pixelStorei(GL_UNPACK_ALIGNMENT, 4); It works the same without this pixelStorei() call right? We don't want to make redundant calls through the context, so this can go away then. Same below.
On 2012/11/20 01:53:07, danakj wrote: > https://chromiumcodereview.appspot.com/11413005/diff/6006/cc/texture_uploader.cc > File cc/texture_uploader.cc (right): > > https://chromiumcodereview.appspot.com/11413005/diff/6006/cc/texture_uploader... > cc/texture_uploader.cc:236: m_context->pixelStorei(GL_UNPACK_ALIGNMENT, 4); > It works the same without this pixelStorei() call right? We don't want to make > redundant calls through the context, so this can go away then. > > Same below. Oh, right. About that: right now nothing else touches GL_UNPACK_ALIGNMENT besides some unittests, but I can't guarantee that, so I was going to leave it in. If we take it out, are y'all CC folk comfortable with assuming that GL_UNPACK_ALIGNMENT will continue to be untouched? We'll get weird video failures again, that might not be easily traceable to this, if this ever changes.
Updated. PTAL.
Thanks for the test! The file is in webkit style and we don't have a stylebot so I'm going to have to fill in for the role and nit the CL to death. But otherwise it seems pretty good. One concern is I don't see a test for a texture width that isn't a multiple of the bytes-per-pixel. https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... File cc/texture_uploader_unittest.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:2: remove this extra newline https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:79: { { onto the previous line. https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:80: case GL_ALPHA: line up the cases with the swich https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:88: { { onto the previous line https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:100: { { up https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:101: case GL_UNSIGNED_BYTE: line up case with switch https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:126: const uint8 *bytes = static_cast<const uint8*>(pixels); uint8* bytes https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:129: for (WGC3Dsizei row = 0; row < height; row += 1) ++row https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:130: { { on previous line https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:131: const uint8 *rowBytes = bytes + (xoffset * bytesPerPixel + (yoffset + row) * stride); uint8* rowBytes https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:204: // Upload a tightly packed 256x256 RGBA texture. How about a test with a non-multiple-of-bytes-per-pixel width texture? 254*256 RGBA or something? https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:206: for (int i = 0; i < 256; i += 1) ++i https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:207: { { up https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:216: for (int i = 0; i < 86; i += 1) ++i https://chromiumcodereview.appspot.com/11413005/diff/1011/cc/texture_uploader... cc/texture_uploader_unittest.cc:217: { { up
All done.
cc LGTM with one last stylenit https://chromiumcodereview.appspot.com/11413005/diff/1015/cc/texture_uploader... File cc/texture_uploader_unittest.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/1015/cc/texture_uploader... cc/texture_uploader_unittest.cc:24: : m_resultAvailable(0), nit: , to next line, below the :
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/11413005/2024
Presubmit check for 11413005-2024 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
media
base
Presubmit checks took 1.9s to calculate.
media/ LGTM
a) When folks want stuff in base, the first question is: How many distinct users do we have for this as a motivation for putting it in base? Most commonly the answer is that it should not be in base. b) The file bits.h seems interested in doing bit-shift like operations. Your code is really about uint32 sizes. It is not clear this would be a the best place to land it (if it was going into base). Could you comment on the (a) part first, before talking about (b). Also, see question below. https://chromiumcodereview.appspot.com/11413005/diff/2024/base/bits.h File base/bits.h (right): https://chromiumcodereview.appspot.com/11413005/diff/2024/base/bits.h#newcode51 base/bits.h:51: return (n + (mul - 1)) / mul * mul; Does this do what you want when: n + mul - 1 > UINT_MAX?
On 2012/11/20 22:54:35, jar wrote: > a) When folks want stuff in base, the first question is: How many distinct users > do we have for this as a motivation for putting it in base? Most commonly the > answer is that it should not be in base. In this case, we've got a user in media (video_frame.cc), and two in cc/ (texture_uploader.cc and its unittest). In addition, it's a common enough idiom for aligning that I would convert other code to use this whenever I see it. > b) The file bits.h seems interested in doing bit-shift like operations. Your > code is really about uint32 sizes. It is not clear this would be a the best > place to land it (if it was going into base). Where else would you suggest? I realize that this is not optimal, but it's the closest to a sort of "math utils" file that I could find. It's using unsigned integer types just so the common case of a round-to-pow2 can be strength-reduced to shfits, or even (if the compiler is smart enough) just a mask.
On 2012/11/20 22:54:35, jar wrote: > a) When folks want stuff in base, the first question is: How many distinct users > do we have for this as a motivation for putting it in base? Most commonly the > answer is that it should not be in base. In this case, we've got a user in media (video_frame.cc), and two in cc/ (texture_uploader.cc and its unittest). In addition, it's a common enough idiom for aligning that I would convert other code to use this whenever I see it. > b) The file bits.h seems interested in doing bit-shift like operations. Your > code is really about uint32 sizes. It is not clear this would be a the best > place to land it (if it was going into base). Where else would you suggest? I realize that this is not optimal, but it's the closest to a sort of "math utils" file that I could find. It's using unsigned integer types just so the common case of a round-to-pow2 can be strength-reduced to shfits, or even (if the compiler is smart enough) just a mask.
https://chromiumcodereview.appspot.com/11413005/diff/2024/base/bits.h File base/bits.h (right): https://chromiumcodereview.appspot.com/11413005/diff/2024/base/bits.h#newcode51 base/bits.h:51: return (n + (mul - 1)) / mul * mul; On 2012/11/20 22:54:35, jar wrote: > Does this do what you want when: > n + mul - 1 > UINT_MAX? It doesn't but I don't really have other suggestions for this, besides increasing the width of the intermediate. O don't believe we'll be rounding to anything excessive -- maybe DCHECK this?
On 2012/11/20 23:13:50, sheu wrote: > https://chromiumcodereview.appspot.com/11413005/diff/2024/base/bits.h > File base/bits.h (right): > > https://chromiumcodereview.appspot.com/11413005/diff/2024/base/bits.h#newcode51 > base/bits.h:51: return (n + (mul - 1)) / mul * mul; > On 2012/11/20 22:54:35, jar wrote: > > Does this do what you want when: > > n + mul - 1 > UINT_MAX? > > It doesn't but I don't really have other suggestions for this, besides > increasing the width of the intermediate. O don't believe we'll be rounding to > anything excessive -- maybe DCHECK this? Never mind this -- I thought of a way to do this more robustly. Updated.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/11413005/5014
https://chromiumcodereview.appspot.com/11413005/diff/2026/cc/texture_uploader.cc File cc/texture_uploader.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/2026/cc/texture_uploader... cc/texture_uploader.cc:35: static uint32 RoundUp(uint32 n, uint32 mul) don't use static here. it's already in anonymous namespace. s/uint32/unsigned int/
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/11413005/13021
Change committed as 168970
https://chromiumcodereview.appspot.com/11413005/diff/13021/cc/texture_uploade... File cc/texture_uploader.cc (right): https://chromiumcodereview.appspot.com/11413005/diff/13021/cc/texture_uploade... cc/texture_uploader.cc:37: return (((n - 1) / mul) * mul) + mul; if n == 0, return 0 first probably. https://chromiumcodereview.appspot.com/11413005/diff/13021/cc/texture_uploade... cc/texture_uploader.cc:46: , m_queryId(0) nit: comma goes at end of previous line, and names are aligned (like you have here, but without the commas). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
