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
[RFC] Java Camera2 View for Android #10081
Conversation
if (image == null) return; | ||
ByteBuffer plane = image.getPlanes()[0].getBuffer(); | ||
Mat yuv = new Mat( h * 3/2, w, CvType.CV_8UC1, plane ); | ||
JavaCamera2Frame tempFrame = new JavaCamera2Frame(yuv,w,h); |
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.
This says that all three planes should be used for YUV_420_888
.
BTW, Please add additional sanity checks (assertions) - for actual Image
format, ByteBuffer
size, etc.
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.
Hmm... OTOH, you're correct (there are 3 planes), but obviously, the planes are contiguous in memory and also mapped in one single chunk, otherwise, the cvtColor
call would cause a segfault.
To implement this in a way that references all 3 planes separately would AFAICT require a new implementation of cvtColor
that can handle 3 separate Mat
objects. Otherwise, you'd need to copy the planes together into a single Mat
and lose the advantage of the zero-copy native buffer.
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 have hal::
conversion functions accepting two separate pointers for Y and UV planes (example), so you can try to use them. You can add more overloaded functions if needed. But it is hard to say how it will work taking in account that each plane has its own PixelStride
parameter which is not supported by current implementations.
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.
According to https://developer.android.com/reference/android/graphics/ImageFormat.html#YUV_420_888 the Y plane will always have a stride of 1, and the U/V planes will have equal stride. I may simply assert that U/V stride is also 1 for the moment (seems to be true for all implementations I've seen so far).
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.
Are you sure U and V have PixelStride=1? Probably it is 2. If your current code worked with COLOR_YUV2RGBA_NV12 conversion by providing only Y pointer, then planes should be located in continuous memory block like this:
YYYY
YYYY
UVUV
Then:
Uptr = Yptr + row_stride * rows
Upixel_stride = 2
Vptr = Uptr + 1
Vpixel_stride = 2
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.
My mistake, you're right. So it's one separate Y plane, directly followed by one interleaved V/U plane.
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.
But in that case, cvtTwoPlaneYUVtoBGR
should probably work?
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.
Probably it should, but we should check these assumptions (via CV_Assert
) before calling any processing code.
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.
@floe , the problem with cvtTwoPlaneYUVtoBGR
is that it does not get wrapped automatically, because it accepts raw C++ pointers. So you will need to write JNI wrapper by hand or wait_for/propose extended cvtColor(InputArray ...)
interface.
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.
Is there any way to check whether the planes are contiguous in memory before passing them to Mat()
? Then the current approach would already be sufficient...
} | ||
} | ||
|
||
boolean cacPreviewSize(final int width, final int height) { |
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.
There is typo (in original code too): calcPreviewSize
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.
Aaah. This is supposed to mean "calc". 🤦♂️ Will fix that.
Please update files filtering for old Android versions here |
I've updated this PR:
The only thing that remains open IMHO is the handling of the RGB conversion here: https://github.com/opencv/opencv/pull/10081/files#diff-980f99834f5223fab7fde83308fc9930R353 AFAICT the cleanest way would be to store Y and U/V plane as two |
Side question: where are the Java wrappers for |
Almost all wrapper for Java/Python bindings are generated automatically from public headers of modules (functions with BTW, |
Another side question: I added
to the initial PR comment, but that still doesn't seem to trigger the Android pack build? |
bump sorry for the noise, but I do need the Android pack to work for testing... |
Nevermind, seems to work now 👍 |
I think this should now be ready for merging. I still can't get the "Android pack" build to trigger reliably, though. Any comments welcome /cc @alalek @mshabunin @vpisarev |
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.
Well done! Works great with Android emulator (x86)!
"Interrupted while createCameraPreviewSession", e); | ||
} | ||
finally { | ||
//mCameraOpenCloseLock.release(); |
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.
What is about this line?
What purpose of this semaphore?
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.
It was already part of the original JavaCameraView
, so I've kept it. However, it's quite possible that this is a kludge from the Camera1 API that's no longer needed for Camera2. I'll see if I can just get rid of it (I'd assume yes ;-)
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.
Agreed.
Looks like locks are no longer needed (frames are processed in "sync" with callback with Camera2 API).
Removed the old semaphore and ran another test; seems to work just as well. Ready for merging IMHO. |
Hm. The Linux OpenCL build failed, looks like a transient build error? |
Build failure is not related to your patch. I run some tests with |
I'll have to test that again. What would be the expected behaviour with the old |
Application activity pause/resume handling should work with app crash/closing |
- re-apply Java code style - imports cleanup - dump expections information
@floe I pushed several updates to this patch. Please take a look. |
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.
@floe Thank you for the contribution!
Thanks for your help! |
@PhilLab sorry for the late reply, have in fact not tried this yet. But since the code is based on |
I have an Android device that only works with
Do you have an idea how to convert all these into a full sized RGBA without copying/merging matrices? |
Going with @floe assumption that native image buffer is contiguous I'm getting desired results with:
|
Ok inspired by JavaCameraView, this one listener handles both image formats :)
|
@ValkA Your code only uses the Y plane which would lead to a gray scale picture. |
It takes also UV in the extra h/2 rows |
Ah, I see. It does seem to be continuous but that is probably a dangerous assumption that can lead to strange issues down the line. |
Yeah, also when lines are padded it has issues |
@ValkA, @floe, @alalek I've done some experiments and it seems that if you use a different size for your image reader than your surface (or perhaps it's just different from the standard 1920x1080) you don't actually get NV21 because of padding between the rows. Furthermore it seems that V always comes before U in memory so just taking the second plane as an |
@alalek I added a review for the PR |
Based on #10050, here's a first alpha implementation of the Camera2 adapter class which should allow zero-copy access to preview image data. Comments very welcome.
Known issues at the moment:
tutorial2-mixedprocessing works with this class as a drop-in replacement for- fixed, was a memory leakJavaCameraView
, but quits after ~ 20-30 seconds (no crash, just exit due to internal camera queue error, perhaps some race condition?)JavaCameraView
andCamera2Renderer
(e.g.JavaCameraFrame
internal class), needs cleanup/refactoringcacPreviewSize(...)
currently buggy, selects smaller preview size than feasible