Closed GoogleCodeExporter closed 9 years ago
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
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
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
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
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
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
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
Original issue reported on code.google.com by
r3gis...@gmail.com
on 12 Mar 2012 at 12:02