-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
Check buffer size when frameWidth * frameHeight bigger than allocated buffer size #21170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…than allocated buffer size - issue nubmer opencv#21149 report, some android 12 device, allocated bufferSize is too small for the retrieve frame, so change logic for debugging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for contribution!
Please take a look on comments below.
if (info.flags & AMEDIACODEC_BUFFER_FLAG_END_OF_STREAM) { | ||
if (frameWidth * frameHeight * 3 / 2 > bufferSize) | ||
{ | ||
if (bufferSize == 3110400 && (frameWidth == 1920 && frameHeight == 1088) || frameWidth == 1088 && frameHeight == 1920) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|| frameWidth == 1088 && frameHeight == 1920
Priority brackets is necessary in condition with &&
/||
mess.
Please use this work around code with shorter conditions to support both 1920x1088 / 1088x1920 cases.
Please add link to the issue for this workaround code.
// WA: https://github.com/opencv/opencv/issues/21149
if (frameWidth * frameHeight * 3 / 2 > bufferSize)
{
if (bufferSize == 3110400 && frameWidth == 1920 && frameHeight == 1088)
{
frameHeight = 1080;
LOGV(...);
}
else if (bufferSize == 3110400 && frameWidth == 1088 && frameHeight == 1920`)
{
frameWidth = 1080;
LOGV(...);
}
else
{
LOGE(...);
return false;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for update!
Please remove unnecessary whitespace: http://pullrequest.opencv.org/buildbot/builders/precommit_docs/builds/33151/steps/whitespace%20opencv/logs/stdio
Also please remove double space on this line:
else if(bufferSize == 3110400 && frameWidth == 1088 && frameHeight == 1920)
^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have whitespace issue: http://pullrequest.opencv.org/buildbot/builders/precommit_docs/builds/33152/steps/whitespace%20opencv/logs/stdio
modules/videoio/src/cap_android_mediandk.cpp:108: trailing whitespace.
+ {
^^^ 3 spaces here
if (info.flags & AMEDIACODEC_BUFFER_FLAG_END_OF_STREAM) | ||
{ | ||
LOGV("output EOS"); | ||
sawOutputEOS = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this code block should go first.
return false;
above should not block sawOutputEOS = true;
flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Thanks your review, I will fix it and repush it!
delete double space about code convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Check buffer size when frameWidth * frameHeight bigger than allocated buffer size
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.