Skip to content
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

MMAL ISP: Bayer 12-bit to I420 produces green-tinted images on second output port #1108

Closed
trya opened this issue Feb 11, 2019 · 7 comments
Closed

Comments

@trya
Copy link

trya commented Feb 11, 2019

Hi,

This issue could be related to #1084. Since commit 8daf963, the second output port of the ISP component produces green-tinted images with RGGB 12-bit Bayer input frames.
9baae76 does not solve this issue.

Minimal use case, which converts a Bayer frame to I420 on both output ports: bayer-isp-issue.zip

Before 8daf963, the converter should produce the same image on both outputs.

@6by9
Copy link

6by9 commented Feb 12, 2019

Agreed it does something funny. All the values are <32, which in the YUV domain is close to green.
Now I need to work out why......

@6by9
Copy link

6by9 commented Feb 13, 2019

So one issue has shown up - I don't enforce that the height on I420 is a multiple of 2, or round up the number of lines in the chroma planes. Your height of 993 causes a minor buffer overflow as it writes the last line of the chroma planes.

@6by9
Copy link

6by9 commented Feb 13, 2019

Found the regression. A shift property had been accidentally reset to 0 instead of 3, so all the pixel values were 1<<3 = x8 smaller than they should be.

A patch has been submitted internally, so should be through with the next rpi-update. Thanks for the report, and particularly for providing a test case.
I wasn't aware that anyone was using the second output of the ISP component, so it's nice to know that my effort there wasn't wasted.

@6by9
Copy link

6by9 commented Feb 13, 2019

BTW your test app is a touch nasty in that it doesn't wait for the buffers to actually be returned and just waits a second. That caught me out as I use an NFS mounted system, and it hadn't finished storing the first image before the timeout hit, so the second one wasn't saved.

@trya
Copy link
Author

trya commented Feb 13, 2019

Thanks for your quick response, it's very much appreciated.

As for the odd height, I usually crop the YUV outputs to even width and height, except for the test app, so that was never a problem to me as far as I can tell.

As for the test app itself, it's a quick and dirty edit from #1084, I didn't bother much with correctness, sorry for that. By the way, how would be a correct way of waiting for buffer release in that case? Waiting on the pool's queue with mmal_queue_wait, setting a callback on the pool with mmal_pool_callback_set or another way?

@6by9
Copy link

6by9 commented Feb 13, 2019

An odd height (or width) is possible for YUV, but it does need the special handling to round the size of the chroma planes up. Whichever way, I ought to trap it in the component.

No problems on it only being a test app - you just caught me out.
Your best bet is probably to disable the port - you're guaranteed that all callbacks will have completed before the mmal_port_disable call completes, at which point it is safe to destroy the pool too.

popcornmix added a commit that referenced this issue Feb 24, 2019
kernel: Rpi 4.19.y V4L2 M2M updates
See: raspberrypi/linux#2857

firmware: camera_subsystem: Clean up disable_camera_led handling

firmware: smservice: Add defines for VPU allocations
smservice: Add support for reporting the supported version to the host

firmware: Camplus: cdi: Remove requirement for calibration functions

firmware: isp: Reinstate lres shift parameter accidentally dropped with gamma changes
See: #1108

firmware: mmal_ril: List all 4 Bayer orders if the IL component says it supports Bayer
popcornmix added a commit to Hexxeh/rpi-firmware that referenced this issue Feb 24, 2019
kernel: Rpi 4.19.y V4L2 M2M updates
See: raspberrypi/linux#2857

firmware: camera_subsystem: Clean up disable_camera_led handling

firmware: smservice: Add defines for VPU allocations
smservice: Add support for reporting the supported version to the host

firmware: Camplus: cdi: Remove requirement for calibration functions

firmware: isp: Reinstate lres shift parameter accidentally dropped with gamma changes
See: raspberrypi/firmware#1108

firmware: mmal_ril: List all 4 Bayer orders if the IL component says it supports Bayer
@trya
Copy link
Author

trya commented Feb 25, 2019

Issue solved with commit 1f4c362.

Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants