Bad access calling convert with not expected target frame height

[GoogleCodeExporter]

This bug is observed using webRTC calling libyuv through this line :
http://code.google.com/p/webrtc/source/browse/trunk/src/modules/video_capture/ma
in/source/video_capture_impl.cc#292

When rotation is 90 or 270 °

My analysis is that they call ConvertToI420
with type => NV21 --matters because will call transpose UV
source buffer => a valid 352x288 Frame
crop x = crop y = 0
width => 352
height => 288,
size => correct frame size for a 352 288 NV21 frame
rotation => 90 ° --- this matters
targetWidth => 352 --- I think that this is the root cause of the problem, in 
unit tests, in this case, it's always the src height that is used (which is 
more normal if we don't want to loose frame infos ;) )
targetHeight => 288 -- same as previous parameter
dstStride => 352
target buffer =>   a valid 352x288 frame


So libyuv will correctly do its job. 
But, when transpose UV will reach the equivalent of 288 in height instead of 
stopping because actual buffer target height is 288, it will continue because 
it expects to fill until the row 352 : since it's transposing a frame with 352 
width. 
And so it will try to write on invalid space of the target buffer.

So it's maybe a bug in the usage made by webRTC. However, maybe some extra 
check or better some silent cropping would be great.

Original issue reported on code.google.com by r3gis...@gmail.com on 12 Mar 2012 at 12:02

[GoogleCodeExporter]

Well after a little bit more investigation I reached the following conclusion 
on what could be done to fix that :
I see 3 different solutions :
 * The first one is to consider that libyuv should not cares about the width x height of the target destination frame size and only sanity check size of y,u and v strides for dest.
If so the signature of the ConvertToI420 should not contains target frame 
width/height as parameters. And dst_width dst_height should be deduced from 
src_width src_height (+crop + rotate).

 * The second one is to consider that libyuv should crop overflow in this case. So it's just about avoiding the y, u, v buffer overflows when transforming from bigger frame. I guess it would be a good robust solution but it's probably not the way webRTC project expect it to work in their video capture use of libyuv.

 * The last one is to consider that this convert method should not only convert, rotate but also scale the frame if the dst_width/dst_height are not in line (or at least smaller) with what should be expected regarding src_width, src_height, crop_x, crop_y and rotation. This solution has the benefit to be generic. But maybe it would break what this level of api is intended for. It also means that convert will depends on scale and I don't know if it's something that is acceptable?

I can try to do a patch if solution 2 or 3 is chosen. 
If no changes are made in webRTC, I would say that solution 3 is the best one : 
it would be consistent with how android preview display a rotated camera 
capture.
So what would be the best solution? 

An illustration of what happens after investigation and possible fixes (because 
a drawing is better than my english ;) ) :
http://www2.r3gis.fr/devs/libyuv_issue_18.png

Original comment by r3gis...@gmail.com on 21 Mar 2012 at 5:00

[GoogleCodeExporter]

i have seen the problem raised by Regis first-hand in the Webrtc demo 
application.
i had a problem when the webrtc demo was started ("StartCall") when the phone 
was in "portrait" orientation, namely an exception due to out-of-bounds access. 
when starting in "landscape" there was no problem, so for now we just stuck to 
landscape only. after the call starts, if i change the orientation, it doesn't 
crash, but the video displayed is rotated 90°.

Original comment by biai...@gmail.com on 22 Mar 2012 at 8:58

[GoogleCodeExporter]

Thanks for the diagram.
The destination width,height is so you can crop to a sub-rectangle of the 
source.  It is expressed in terms of unrotated source.
It should normally be smaller than the source, but I may allow larger, which 
would pad with black for letter boxing.  Perhaps I should enforce smaller 
destination width/height than source, which would catch callers getting these 
confused in the rotated case.

Its difficult to sanity check the strides.  Best practice is to allocate YUV 
with rows and planes aligned to cache lines for efficiency.  Stride is used to 
round pixels to even bytes for odd sizes image.  You can also point libyuv 
functions at an existing image and use it to render into it.  Stride on source 
to clip a rectangle and stride on destination to describe the target frame 
buffer size.
Stride can also be negative on either source or destination to allow vertical 
flipping.
Stride should usually be >= destination row widths.  But I've seen 0 for stride 
to repeat rows or do one row.

A robust solution would include frame sizes to ensure pointers stay within a 
range of memory.  That would be cumbersome at NV12Rotate level, but 
ConvertToI420 could take buffer sizes.

For I420Scale, I think its feasible to add rotation by 90/180/270 in the future 
with minimal code/performance impact compared to general purpose scaling.  It 
supports negative height and I'd like to add negative width which gives 180 
rotation. A new parameter is needed for 90/270. But so far I don't plan to 
expose scaling for all camera formats.

Original comment by fbarch...@google.com on 29 Mar 2012 at 12:32

[GoogleCodeExporter]

Ok, it makes sense. I didn't thought about the fact stride could be allocated 
independently of dest width/height, but that's right it's some possible usage 
of the lib.

So, maybe this is actually not a bug in libyuv but more something in the usage 
of libyuv made by webRTC. 
I will open an issue on webRTC : I actually already have a patch for webRTC 
that does thing in two steps if they want to convert + rotate keeping same 
width x height : it use convert with rotation but correctly allocated 
destination frame and inverted width x height, and then in a second time use 
scale (of libyuv) to transform w x h to h x w. 
It's not so much code to add and should not affect performance on their side. 
So it's probably the best solution to solve the initial problem if it's not the 
purpose of convert to do that.

Then, indeed, for libyuv it's only a robustness/buffer overflow check problem. 
But I think that it would be also acceptable if libyuv is not robust in this 
case if the input arguments does not allow to check robustness. 
However, if so I think that a warning should be added in comments/doc of each 
concerned method about the fact it's up to who uses the method to do sanity 
check on what he allocates and passes to the lib. So if it's not in the code it 
should be added as a doc info on usage of the lib. And delegates check of 
buffer overflow to caller. Maybe some possible helper would be an optional 
method to do the sanity check of buffers as a standalone method, that could be 
call by lib user in case they have variable input and want to convert to fix 
output. But in webRTC case, it's not necessary since the output buffer size 
could be totally deduced from input.

Original comment by r3gis...@gmail.com on 29 Mar 2012 at 10:27

[GoogleCodeExporter]

For the record, an issue has been created on webrtc issue list :
http://code.google.com/p/webrtc/issues/detail?id=424

There is also a patch in my series of quilt patches to fix this problem on 
webRTC :
http://code.google.com/p/csipsimple/source/browse/trunk/CSipSimple#CSipSimple%2F
jni%2Fwebrtc%2Fpatches
It's patch 003libyuv_misusage_rotation.diff (you may also need patch 002 to 
have things building with latest webrtc code due to a regression in folder 
layout vs android.mk files).

It fixes the issue for my usage of webRTC and I think that it will also fix the 
webrtc's demo "StartCall" as well. (and maybe it also fixes for other platforms 
with rotated camera, since apparently the problem was not only linked to 
android).

Original comment by r3gis...@gmail.com on 1 Apr 2012 at 6:27

[GoogleCodeExporter]

thanks... a quick look at your CL's look ok to me.

An increasing number of (new) users tells me that usage of libyuv is easily 
wrong and leads to issues, so I'll try to
1. document
2. fix known callers (webrtc)
3. provide information API that helps caller know everything about a format
ie how many bytes to allocate for a (FOURCC, width, height)

Original comment by fbarch...@google.com on 14 Apr 2012 at 1:43

[GoogleCodeExporter]

No immediate action on libyuv.  Once webrtc bugs are fixed, libyuv should be 
accessed in a reasonable way for most users.

Original comment by fbarch...@google.com on 5 Jun 2012 at 12:25

• Changed state: WontFix