Skip to content

Fix Memory Issue#20509

Closed
saikatnanda wants to merge 5 commits intoopencv:masterfrom
saikatnanda:master
Closed

Fix Memory Issue#20509
saikatnanda wants to merge 5 commits intoopencv:masterfrom
saikatnanda:master

Conversation

@saikatnanda
Copy link
Copy Markdown

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

The code was crashing on buffer.insert() line#330 on Android 11 Emulator with Camera2 NDK. This is tested with a C++ Test Application.

It looks like the U & V indexes are revered and the YUVPlaner check condition is having an assignment operator than ==.
This fix satisfies the YUV buffer memory layout expectation as in https://en.wikipedia.org/wiki/YUV#/media/File:Yuv420.svg

@saikatnanda
Copy link
Copy Markdown
Author

saikatnanda commented Aug 5, 2021

@komakai I figure you were the primary contributor on this feature. It will be helpful if you please let me know in case I am missing something here.

AImage_getPlaneData(image.get(), 1, &vPixel, &vLen);
AImage_getPlaneData(image.get(), 2, &uPixel, &uLen);
AImage_getPlaneData(image.get(), 1, &uPixel, &vLen);
AImage_getPlaneData(image.get(), 2, &vPixel, &uLen);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you also want to swap vLen with uLen ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I missed that. They were of same length, so I did not catch it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@komakai I made the changes. Can please suggest how to take this PR further? I am not sure why the default build has failed. I can probably initiate a retest is that is something holding us back.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saikatnanda I'm also not sure why the build is failing - will try and take a look when I get a minute. In the meantime could you rollup your changes into a single commit? Also it might be worth rebasing onto the latest master - sometimes that solves weird build failures.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will then rebase and will create a fresh PR to look better.

fourCC = FOURCC_NV21;
}
} else if ( (uvPixelStride == 1) && (vPixel = uPixel + uLen) && (yLen == frameWidth * frameHeight) && (uLen == yLen / 4) && (vLen == uLen) ) {
} else if ( (uvPixelStride == 1) && (vPixel == uPixel + uLen) && (yLen == frameWidth * frameHeight) && (uLen == yLen / 4) && (vLen == uLen) ) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thanks for finding that.

@saikatnanda
Copy link
Copy Markdown
Author

bump!!

@saikatnanda saikatnanda deleted the branch opencv:master August 10, 2021 01:26
@saikatnanda saikatnanda deleted the master branch August 10, 2021 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants