search

2753536587 / libyuv

Automatically exported from code.google.com/p/libyuv
0 stars 0 forks source link

ConvertToARGB() from UYVY: access violation if src_height < 0 #76

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. ConvertToARGB(InputBuffer, 0,
        OutputBuffer, 504*4,
        0, 0, 504, -504, 504, 504,
        kRotate0, FOURCC_UYVY)
2. Repeat call 2 or 3 times to reproduce access violation

What is the expected output? What do you see instead?

Expected: work. Instead: access violation.

What version of the product are you using? On what operating system?

r325 on Windows (VC++)

Please provide any additional information below.

Seems to work fine if height is not inverted.

Original issue reported on code.google.com by alt...@gmail.com on 29 Aug 2012 at 11:04

GoogleCodeExporter commented 9 years ago 
Moreover, even when height is positive, there are huge values differences in 
output buffer between consecutive calls with the same input data

Original comment by alt...@gmail.com on 29 Aug 2012 at 12:11

GoogleCodeExporter commented 9 years ago 
Same bug with FOURCC_YUY2

Original comment by alt...@gmail.com on 29 Aug 2012 at 2:56

GoogleCodeExporter commented 9 years ago 
Thanks for the report.  Your parameters look ok.
It will end up calling the function:

YUY2ToARGB(src, aligned_src_width * 2,      
           dst_argb, argb_stride,           
           dst_width, inv_dst_height);      

with -504 for height.  Which seems to support negative height correctly.
// Negative height means invert the image.                   
if (height < 0) {                                            
  height = -height;                                          
  src_yuy2 = src_yuy2 + (height - 1) * src_stride_yuy2;      
  src_stride_yuy2 = -src_stride_yuy2;                        
}                                                            

I'll have to write a test to see where its going wrong.
On your end, make sure you allocated enough memory
width * height * 2 for UYVY and width * height * 4 for ARGB where width and 
height are positive values.

As a work around, you can call the functions with the OutputBuffer pointing to 
the last row and negative stride. e.g.
ConvertToARGB(InputBuffer, 0,
        OutputBuffer + 504 * 4 * 503, -504*4,
        0, 0, 504, -504, 504, 504,
        kRotate0, FOURCC_UYVY)

Original comment by fbarch...@google.com on 30 Aug 2012 at 12:20

GoogleCodeExporter commented 9 years ago 
Wrote unittests and caught the bug.  Infact the bug could happen on odd 
positive heights too, but the odds of a memory exception would be higher with a 
negative height.
It related to the UYVY/YUY2 subsampling taking the row and row + 1.  But on the 
odd rows, this would oversample beyond the end of the buffer.
The new code is unrolled to do 2 rows at a time.
The issue seems to be isolated to just those 2 functions.
The YUY2 To I420 already had the correct solution.

c:\src\libyuv\trunk>build\release\libyuv_unittest.exe 
--gtest_catch_exceptions=0 --gtest_filter=*Invert*
Note: Google Test filter = *Invert*
[==========] Running 58 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 58 tests from libyuvTest
[ RUN      ] libyuvTest.I420ToARGBInvert_OptVsC
[       OK ] libyuvTest.I420ToARGBInvert_OptVsC (860 ms)
[ RUN      ] libyuvTest.I420ToBGRAInvert_OptVsC
[       OK ] libyuvTest.I420ToBGRAInvert_OptVsC (867 ms)
[ RUN      ] libyuvTest.I420ToABGRInvert_OptVsC
[       OK ] libyuvTest.I420ToABGRInvert_OptVsC (847 ms)
[ RUN      ] libyuvTest.I420ToRAWInvert_OptVsC
[       OK ] libyuvTest.I420ToRAWInvert_OptVsC (1020 ms)
[ RUN      ] libyuvTest.I420ToRGB24Invert_OptVsC
[       OK ] libyuvTest.I420ToRGB24Invert_OptVsC (1019 ms)
[ RUN      ] libyuvTest.I420ToRGB565Invert_OptVsC
[       OK ] libyuvTest.I420ToRGB565Invert_OptVsC (1478 ms)
[ RUN      ] libyuvTest.I420ToARGB1555Invert_OptVsC
[       OK ] libyuvTest.I420ToARGB1555Invert_OptVsC (1589 ms)
[ RUN      ] libyuvTest.I420ToARGB4444Invert_OptVsC
[       OK ] libyuvTest.I420ToARGB4444Invert_OptVsC (1248 ms)
[ RUN      ] libyuvTest.I411ToARGBInvert_OptVsC
[       OK ] libyuvTest.I411ToARGBInvert_OptVsC (869 ms)
[ RUN      ] libyuvTest.I422ToARGBInvert_OptVsC
[       OK ] libyuvTest.I422ToARGBInvert_OptVsC (863 ms)
[ RUN      ] libyuvTest.I444ToARGBInvert_OptVsC
[       OK ] libyuvTest.I444ToARGBInvert_OptVsC (894 ms)
[ RUN      ] libyuvTest.I420ToYUY2Invert_OptVsC
[       OK ] libyuvTest.I420ToYUY2Invert_OptVsC (248 ms)
[ RUN      ] libyuvTest.I420ToUYVYInvert_OptVsC
[       OK ] libyuvTest.I420ToUYVYInvert_OptVsC (42 ms)
[ RUN      ] libyuvTest.I420ToV210Invert_OptVsC
[       OK ] libyuvTest.I420ToV210Invert_OptVsC (1767 ms)
[ RUN      ] libyuvTest.I420ToI400Invert_OptVsC
[       OK ] libyuvTest.I420ToI400Invert_OptVsC (139 ms)
[ RUN      ] libyuvTest.I420ToBayerBGGRInvert_OptVsC
[       OK ] libyuvTest.I420ToBayerBGGRInvert_OptVsC (1127 ms)
[ RUN      ] libyuvTest.I420ToBayerRGGBInvert_OptVsC
[       OK ] libyuvTest.I420ToBayerRGGBInvert_OptVsC (1115 ms)
[ RUN      ] libyuvTest.I420ToBayerGBRGInvert_OptVsC
[       OK ] libyuvTest.I420ToBayerGBRGInvert_OptVsC (1116 ms)
[ RUN      ] libyuvTest.I420ToBayerGRBGInvert_OptVsC
[       OK ] libyuvTest.I420ToBayerGRBGInvert_OptVsC (1118 ms)
[ RUN      ] libyuvTest.NV12ToARGBInvert_OptVsC
[       OK ] libyuvTest.NV12ToARGBInvert_OptVsC (824 ms)
[ RUN      ] libyuvTest.NV21ToARGBInvert_OptVsC
[       OK ] libyuvTest.NV21ToARGBInvert_OptVsC (822 ms)
[ RUN      ] libyuvTest.NV12ToRGB565Invert_OptVsC
[       OK ] libyuvTest.NV12ToRGB565Invert_OptVsC (1435 ms)
[ RUN      ] libyuvTest.NV21ToRGB565Invert_OptVsC
[       OK ] libyuvTest.NV21ToRGB565Invert_OptVsC (1436 ms)
[ RUN      ] libyuvTest.ARGBToI420Invert_OptVsC
[       OK ] libyuvTest.ARGBToI420Invert_OptVsC (627 ms)
[ RUN      ] libyuvTest.BGRAToI420Invert_OptVsC
[       OK ] libyuvTest.BGRAToI420Invert_OptVsC (625 ms)
[ RUN      ] libyuvTest.ABGRToI420Invert_OptVsC
[       OK ] libyuvTest.ABGRToI420Invert_OptVsC (636 ms)
[ RUN      ] libyuvTest.RAWToI420Invert_OptVsC
[       OK ] libyuvTest.RAWToI420Invert_OptVsC (798 ms)
[ RUN      ] libyuvTest.RGB24ToI420Invert_OptVsC
[       OK ] libyuvTest.RGB24ToI420Invert_OptVsC (805 ms)
[ RUN      ] libyuvTest.RGB565ToI420Invert_OptVsC
[       OK ] libyuvTest.RGB565ToI420Invert_OptVsC (999 ms)
[ RUN      ] libyuvTest.ARGB1555ToI420Invert_OptVsC
[       OK ] libyuvTest.ARGB1555ToI420Invert_OptVsC (1104 ms)
[ RUN      ] libyuvTest.ARGB4444ToI420Invert_OptVsC
[       OK ] libyuvTest.ARGB4444ToI420Invert_OptVsC (847 ms)
[ RUN      ] libyuvTest.ARGBToI422Invert_OptVsC
[       OK ] libyuvTest.ARGBToI422Invert_OptVsC (822 ms)
[ RUN      ] libyuvTest.YUY2ToI420Invert_OptVsC
[       OK ] libyuvTest.YUY2ToI420Invert_OptVsC (318 ms)
[ RUN      ] libyuvTest.UYVYToI420Invert_OptVsC
[       OK ] libyuvTest.UYVYToI420Invert_OptVsC (320 ms)
[ RUN      ] libyuvTest.V210ToI420Invert_OptVsC
[       OK ] libyuvTest.V210ToI420Invert_OptVsC (1144 ms)
[ RUN      ] libyuvTest.I400ToI420Invert_OptVsC
[       OK ] libyuvTest.I400ToI420Invert_OptVsC (176 ms)
[ RUN      ] libyuvTest.BayerBGGRToI420Invert_OptVsC
[       OK ] libyuvTest.BayerBGGRToI420Invert_OptVsC (2534 ms)
[ RUN      ] libyuvTest.BayerRGGBToI420Invert_OptVsC
[       OK ] libyuvTest.BayerRGGBToI420Invert_OptVsC (2379 ms)
[ RUN      ] libyuvTest.BayerGBRGToI420Invert_OptVsC
[       OK ] libyuvTest.BayerGBRGToI420Invert_OptVsC (2394 ms)
[ RUN      ] libyuvTest.BayerGRBGToI420Invert_OptVsC
[       OK ] libyuvTest.BayerGRBGToI420Invert_OptVsC (2542 ms)
[ RUN      ] libyuvTest.ARGBToARGBInvert_OptVsC
[       OK ] libyuvTest.ARGBToARGBInvert_OptVsC (499 ms)
[ RUN      ] libyuvTest.ARGBToBGRAInvert_OptVsC
[       OK ] libyuvTest.ARGBToBGRAInvert_OptVsC (533 ms)
[ RUN      ] libyuvTest.ARGBToABGRInvert_OptVsC
[       OK ] libyuvTest.ARGBToABGRInvert_OptVsC (533 ms)
[ RUN      ] libyuvTest.ARGBToRAWInvert_OptVsC
[       OK ] libyuvTest.ARGBToRAWInvert_OptVsC (517 ms)
[ RUN      ] libyuvTest.ARGBToRGB24Invert_OptVsC
[       OK ] libyuvTest.ARGBToRGB24Invert_OptVsC (518 ms)
[ RUN      ] libyuvTest.ARGBToRGB565Invert_OptVsC
[       OK ] libyuvTest.ARGBToRGB565Invert_OptVsC (807 ms)
[ RUN      ] libyuvTest.ARGBToARGB1555Invert_OptVsC
[       OK ] libyuvTest.ARGBToARGB1555Invert_OptVsC (893 ms)
[ RUN      ] libyuvTest.ARGBToARGB4444Invert_OptVsC
[       OK ] libyuvTest.ARGBToARGB4444Invert_OptVsC (606 ms)
[ RUN      ] libyuvTest.BGRAToARGBInvert_OptVsC
[       OK ] libyuvTest.BGRAToARGBInvert_OptVsC (525 ms)
[ RUN      ] libyuvTest.ABGRToARGBInvert_OptVsC
[       OK ] libyuvTest.ABGRToARGBInvert_OptVsC (528 ms)
[ RUN      ] libyuvTest.RAWToARGBInvert_OptVsC
[       OK ] libyuvTest.RAWToARGBInvert_OptVsC (686 ms)
[ RUN      ] libyuvTest.RGB24ToARGBInvert_OptVsC
[       OK ] libyuvTest.RGB24ToARGBInvert_OptVsC (686 ms)
[ RUN      ] libyuvTest.RGB565ToARGBInvert_OptVsC
[       OK ] libyuvTest.RGB565ToARGBInvert_OptVsC (575 ms)
[ RUN      ] libyuvTest.ARGB1555ToARGBInvert_OptVsC
[       OK ] libyuvTest.ARGB1555ToARGBInvert_OptVsC (672 ms)
[ RUN      ] libyuvTest.ARGB4444ToARGBInvert_OptVsC
[       OK ] libyuvTest.ARGB4444ToARGBInvert_OptVsC (486 ms)
[ RUN      ] libyuvTest.YUY2ToARGBInvert_OptVsC
[       OK ] libyuvTest.YUY2ToARGBInvert_OptVsC (1093 ms)
[ RUN      ] libyuvTest.UYVYToARGBInvert_OptVsC
[       OK ] libyuvTest.UYVYToARGBInvert_OptVsC (1106 ms)
[ RUN      ] libyuvTest.M420ToARGBInvert_OptVsC
[       OK ] libyuvTest.M420ToARGBInvert_OptVsC (827 ms)
[----------] 58 tests from libyuvTest (54065 ms total)

[----------] Global test environment tear-down
[==========] 58 tests from 1 test case ran. (54069 ms total)
[  PASSED  ] 58 tests.

Original comment by fbarch...@chromium.org on 30 Aug 2012 at 7:34

GoogleCodeExporter commented 9 years ago 
Thank you very much.
The additional unit tests are a good thing too.
I will test again as soon as the changes appears on SVN

Original comment by alt...@gmail.com on 30 Aug 2012 at 8:01

GoogleCodeExporter commented 9 years ago 
r333 fixes the issue, which had to do with subsampling, which it shouldn't have 
been doing, since it goes ARGB, but I420.

Keeping bug open as I may do a followup CL to specialize a YUY2ToUV with no 
subsampling which will be a little faster.

Original comment by fbarch...@chromium.org on 30 Aug 2012 at 8:41

GoogleCodeExporter commented 9 years ago 
Caveat r333 unittest is okay on Visual C but has syntax error for GCC.

This is the review for the improved version of YUY2ToARGB:
https://webrtc-codereview.appspot.com/763006

in r333 the UV subsampled with itself.  And off the end of the image.
My first fix was to do 2 rows as a time, which improved performance, but then I 
realized there is not reason for down sampling YUY2 from 422 to 420 before 
going to ARGB.
So in r333 it passes 0 for stride which UV subsampled the rows with itself.
Its higher quality than the original version (r325), but a waste of time still 
going thru the motions of subsampling - same speed as the original.

In the proposed CL r334 removes the stride parameter and does not subsample.

Original comment by fbarch...@chromium.org on 30 Aug 2012 at 10:32

GoogleCodeExporter commented 9 years ago 
r336 fixes all known issues.
UYVY and YUY2 should be faster and higher quality now.
Thanks for reporting the bug!

Original comment by fbarch...@chromium.org on 30 Aug 2012 at 9:28