Setting opaque still port encoding breaks subsequent full-res YUV captures

[waveform80]

While debugging waveform80/picamera#73 I've run into some behaviour which I suspect may be a bug in the firmware (or at least in MMAL - my apologies if I've got the wrong subject here!).

Firstly a little bit of background: picamera's a bit of an all-rounder so on occasion it has a need to change the encoding of the camera's still port. For example, to capture JPEGs with full Exif meta-data, the port needs to have "opaque" encoding, while capturing YUV requires the port to have "I420" encoding.

Several users reported receiving ENOMEM when attempting YUV captures at full resolution (2592x1944). While I could reproduce this behaviour with picamera, I couldn't (initially) reproduce it with raspiyuv. However, after several hours debugging I've finally managed to track it down: if the still port is ever set to "opaque" encoding, even if it is then reset to I420 encoding, subsequent captures with fail with ENOMEM.

To demonstrate this in raspiyuv, apply the following patch to RaspiStillYUV.c:

diff --git a/host_applications/linux/apps/raspicam/RaspiStillYUV.c b/host_applications/linux/apps/raspicam/RaspiStillYUV.c
index 9b30a31..78f5c2a 100644
--- a/host_applications/linux/apps/raspicam/RaspiStillYUV.c
+++ b/host_applications/linux/apps/raspicam/RaspiStillYUV.c
@@ -584,6 +584,10 @@ static MMAL_STATUS_T create_camera_component(RASPISTILLYUV_STATE *state)

    still_port->buffer_num = still_port->buffer_num_recommended;

+   still_port->format->encoding = MMAL_ENCODING_OPAQUE;
+   status = mmal_port_format_commit(still_port);
+
+   still_port->format->encoding = MMAL_ENCODING_I420;
    status = mmal_port_format_commit(still_port);

    if (status)

This simply tweaks raspiyuv so that, after configuring the still port it switches it to opaque encoding, then back to I420 encoding. Rebuild raspiyuv, then run it with the following command line:

raspiyuv -v -o foo.data

Which should produce the following output:

raspiyuv Camera App v1.3.3

Width 2592, Height 1944, filename foo.data
Time delay 5000, Timelapse 0
Preview Yes, Full screen Yes
Preview window 0,0,1024,768
Opacity 255
Sharpness 0, Contrast 0, Brightness 50
Saturation 0, ISO 0, Video Stabilisation No, Exposure compensation 0
Exposure Mode 'auto', AWB Mode 'auto', Image Effect 'none'
Metering Mode 'average', Colour Effect Enabled No with U = 128, V = 128
Rotation 0, hflip No, vflip No
ROI x 0.000000, y 0.000000, w 1.000000 h 1.000000
Camera component done
Starting component connection stage
Opening output file foo.data
Enabling camera still output port
mmal: mmal_vc_port_enable: failed to enable port vc.ril.camera:out:2(I420): ENOMEM
mmal: mmal_port_enable: failed to enable port vc.ril.camera:out:2(I420)(0xdbe290) (ENOMEM)
mmal: Failed to setup camera output
mmal: Out of memory
Closing down
Close down completed, all components disconnected, disabled and destroyed

mmal: Failed to run camera app. Please check for firmware updates

This is essentially the same error I'm seeing in picamera. The only workaround I can think of at the moment is to destroy and recreate the MMAL camera component whenever anyone performs an operation that requires a port encoding switch, but that's awfully complicated as it'll involve storing and restoring all the state associated with it (brightness, exposure mode, resolution, etc. etc. etc.)

Is there any chance this could be fixed at the firmware level instead?

[waveform80]

I should add that subsequent captures below full resolution (e.g. 720p or 1080p) seem to work happily, although I haven't (yet) exhaustively tested to find out exactly what resolution(s) between 1080p and full may cause the ENOMEM state.

[waveform80]

A couple of extra data points:

The test detailed above was carried out with firmware revisions 679 and 680. When GPU memory split is set to the default of 128Mb, the test fails. When the split is set to 256Mb (on a model B, obviously) the test succeeds (again, under both firmware revisions).

[6by9]

It's possible that there is a memory leak, but I'd . I420 2592x1944 is 7.5MB per buffer, so not insignificant. Opaque buffers are (IIRC) about 64bytes! Even dropping to 1080P means the drops the buffer size to just over 3MB.

Ah, daft though - how many buffers are you asking for? You're setting still_port->buffer_num BEFORE you change the buffer format. IIRC buffer_num_recommended for I420 will be 1. buffer_num_recommended for opaque is 20. 20 buffers at 7.5MB each isn't going to fit into 128MB. buffer_num only needs to be set before enabling the port, not before port_format_commit, and the commit may change the recommended/min value.

[waveform80]

Interesting - I've just checked the code and although buffers are set after port_format_commit when an instance of PiCamera is first created, they're not modified at all when the still port's encoding is later modified. If the port_format_commit can change a port's buffer_num or the size of the buffers (as well as the recommended/min value) then that could well be the issue. I'll have a play a bit later this evening after work.

[6by9]

port_format_commit shouldn't change the buffer_num or buffer_size fields, but it will change buffer_num_min, buffer_num_recommended and buffer_size_min to reflect the requirements of the new format Buffer size for RGBA > RGB565 > YUV > opaque - it'd be silly never to change buffer_size_min to reflect this. I think buffer_num_min is 1 for image buffers and 3 for opaque. recommended changes to probably 10 for opaque. (Sorry, I'm looking at another tree which has related to the Pi tree but has moved on a lot since)

If buffer_num < buffer_num_min or buffer_size < buffer_size_min at the point you call mmal_port_enable then you'll get MMAL_EINVAL back (https://github.com/raspberrypi/userland/blob/master/interface/mmal/core/mmal_port.c#L478)

[waveform80]

Some interesting results from playing with this:

Firstly, I think we're on the right track. I forced a few port encoding switches manually in picamera and watched the resulting buffer settings. Switching it to opaque and back to I420 did indeed result in buffer_num changing from 1 (which looks like the default for I420) to 20 (which seems to be the opaque min), while the buffer_size stayed constant (in the 7.5Mb region).

I could swear the picamera code doesn't touch the buffer configuration (which would contradict your assertion that port_format_commit doesn't change buffer_num - it seems to change it to the min if it's currently too low, but otherwise leave it untouched), but I'll spend some time at the weekend single-stepping the code to make sure. There also seemed to be something odd going on with the recommended values (buffer_num_recommended started off at 1 for I420, changed to 20 to opaque, and then stayed at 20 when I switched back to I420; buffer_num_min seemed to be sensible though), but I'll do a few more tests to make sure my eyes aren't deceiving me there too.

Anyway, definitely looks like you hit the nail on the head with the cause - should have this cleared up properly for 1.5!

[6by9]

I only use MMAL, I didn't write it ;-)

I thought that port_format_commit didn't change things, but it does appear to set the size and num to at least the minimum - https://github.com/raspberrypi/userland/blob/master/interface/mmal/core/mmal_port.c#L327 port_enable then checks that the values are at least the minimum and errors if not - https://github.com/raspberrypi/userland/blob/master/interface/mmal/core/mmal_port.c#L478 That works fine for increasing the buffer size/num, but not decreasing them :-(

Sorry, the code that sets up the buffer size/num for opaque buffers is GPU side so I can't link to it. There does seem to be some logic missing on setting the buffer_num_recommended when switching off opaque buffers.

So it seems that selecting buffer_num = buffer_num_min, and buffer_size = buffer_size_recommended or buffer_size_min after each format_commit, but before the enable would be the most sensible approach. There shouldn't be a need to commit those values after the change. I'm just looking at our main code branch, and find: status = mmal_port_format_commit( port ); if ( port->format->encoding == MMAL_ENCODING_OPAQUE ) port->buffer_num = port->buffer_num_recommended; else port->buffer_num = vcos_max(IDEAL_NUMBER_FOR_REST_OF_SYSTEM, port->buffer_num_min); and the pool is allocated with buffers of port->buffer_size_min. :-) I'll raise an internal issue to look into the buffer_num_recommended, as that is weird behaviour.

[waveform80]

Many thanks for looking into this - what you've described is definitely the behaviour I'm seeing. I'll implement your suggested handling of buffer_num and buffer_size in picamera for 1.5.

P.S. My apologies if I implied your code was at fault - not my intention!

[6by9]

Old issue, but I'm hoping I've fixed this in the latest firmware. https://github.com/Hexxeh/rpi-firmware/commit/a1621d67b20f04acdac602d0b24923ec3cdbfb66 "firmware: MMAL: reset buffer recommended values on switching back to raw pixels"

[Ruffio]

@waveform80 can this issue be closed?

[waveform80]

I'll open a ticket on picamera to remind me to test this before the next release; I'm a bit busy with picraft (and dealing with a 2 month old!) at the moment so it might be a week or two before I get to this but if it's in the picamera issue tracker I won't forget it :)

[6by9]

As referenced above, I did put in a change for this that was released in https://github.com/Hexxeh/rpi-firmware/commit/a1621d67b20f04acdac602d0b24923ec3cdbfb66 Hopefully buffer_num_recommended and buffer_size_recommended are now reset to sensible values when switching back from opaque mode.

[waveform80]

Sorry it's taken me so long to get around to testing this (she's now 10 months old!). The buffer_num_recommended fix has certainly changed something - it now resets to lower values ... but I'm not sure they're "sensible" as such. Here's the output from a command line session demonstrating the issue:

Python 2.7.9 (default, Mar  8 2015, 00:52:26) 
[GCC 4.9.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import picamera
>>> camera = picamera.PiCamera()
>>> camera.resolution
(1280, 800)
>>> camera._camera[0].output[2][0].format[0].encoding
MMAL_FOURCC('I420')
>>> camera._camera[0].output[2][0].buffer_num
1L
>>> camera._camera[0].output[2][0].buffer_size
1536000L
>>> camera._camera[0].output[2][0].buffer_num_min
1L
>>> camera._camera[0].output[2][0].buffer_num_recommended
1L
>>> camera._camera[0].output[2][0].buffer_size_min
1536000L
>>> camera._camera[0].output[2][0].buffer_size_recommended
1536000L
>>> camera.capture('/dev/null', format='jpeg')
>>> camera._camera[0].output[2][0].format[0].encoding
MMAL_FOURCC('OPQV')
>>> camera._camera[0].output[2][0].buffer_num
10L
>>> camera._camera[0].output[2][0].buffer_size
128L
>>> camera._camera[0].output[2][0].buffer_num_min
3L
>>> camera._camera[0].output[2][0].buffer_num_recommended
10L
>>> camera._camera[0].output[2][0].buffer_size_min
128L
>>> camera._camera[0].output[2][0].buffer_size_recommended
128L
>>> camera.capture('/dev/null', format='yuv')
>>> camera._camera[0].output[2][0].format[0].encoding
MMAL_FOURCC('I420')
>>> camera._camera[0].output[2][0].buffer_num
1L
>>> camera._camera[0].output[2][0].buffer_size
1536000L
>>> camera._camera[0].output[2][0].buffer_num_min
1L
>>> camera._camera[0].output[2][0].buffer_num_recommended
0L
>>> camera._camera[0].output[2][0].buffer_size_min
1536000L
>>> camera._camera[0].output[2][0].buffer_size_recommended
1536000L

Just to explain the output above: camera._camera is actual MMAL camera component. For those familiar with C, all the [0]'s are just Python's way of de-referencing pointers (which would work in C, but -> is a bit more "normal"). So the above session:

1. Sets up the camera, and checks the defaults that picamera sets on the still port (I420 encoding, 1 buffer of 1.5Mb - derived from buffer_num_min and buffer_size_recommended as discussed above).
2. Captures a JPEG writing the output to /dev/null. This forces the still port to opaque encoding and we see sensible values on buffer_num_recommended and buffer_size_recommended.
3. Captures unencoded YUV writing the output to /dev/null again. This forces the still port back to I420 encoding and we see a sensible value for buffer_size_recommended, but buffer_num_recommended is 0?

The tests were performed immediately after updating the firmware with sudo rpi-update and the firmware revision was reported by uname -a as 858. Does the 0 have a special meaning (i.e. go and use the "min" field)? Anyway, picamera's still working happily using buffer_num_min and buffer_size_recommended.

[6by9]

(I can relate - mine has her 1st birthday this week!)

That sounds like we still have an issue. At the IL level (hiding below MMAL), a buffer count of 0 does have a special meaning to signify proprietary tunnelling mode. That is repurposed for MMAL opaque images. I've avoided Python having had a very bad introduction back in about 2005 (don't ask), but if I can reproduce just using the instructions above then that should be easy enough to debug it.

[JamesH65]

Ping...

[JamesH65]

@6by9 Has there been any change on this? Close as wont fix? Something else?

[JamesH65]

This issue will be closed within 30 days unless further interactions are posted. If you wish this issue to remain open, please add a comment. A closed issue may be reopened if requested.