hello_pi: Add hello_mmal_encode example #349

sconemad · 2016-10-27T15:00:35Z

An example showing how to use the image encoder via MMAL to encode a test image into various formats.

6by9

Thanks for the patch - having an example is useful.

I haven't tried to run it, and my comments are generally minor.

@popcornmix Do we want a new directory for MMAL based examples? All of the IL based ones really ought to be ported to MMAL as well (yet another task).
Also hello_encode is for video_encode, so do we want to distinguish this image_encode test from that?

6by9 · 2016-10-27T15:22:33Z

host_applications/linux/apps/hello_pi/hello_mmal_encode/mmal_encode.c

+
+  fprintf(stdout, "Encoding test image %s\n", filename);
+
+  if (mmal_wrapper_create(&mmal, MMAL_COMPONENT_DEFAULT_IMAGE_ENCODER)


mmal as a variable name is very ambiguous. "encoder" or "component"?

6by9 · 2016-10-27T15:24:29Z

host_applications/linux/apps/hello_pi/hello_mmal_encode/mmal_encode.c

+  portIn = mmal->input[0];
+  mmal->status = MMAL_SUCCESS;
+
+  if (portIn->is_enabled) {


I'm not aware of any way for components to have enabled ports this soon after creation. Redundant? Or is this copy/paste from other examples? (In which case they ought to be amended).

Yes, it doesn't seem to be necessary in this case. In general it might be though, if people are reconfiguring an existing encoder, so perhaps it would be good to show this. How about if the encoder is created in main(), then reconfigured for each encoding?

6by9 · 2016-10-27T15:24:51Z

host_applications/linux/apps/hello_pi/hello_mmal_encode/mmal_encode.c

+
+  portOut = mmal->output[0];
+
+  if (portOut->is_enabled) {


Ditto - I don't see how this can happen.

6by9 · 2016-10-27T15:31:25Z

host_applications/linux/apps/hello_pi/hello_mmal_encode/mmal_encode.c

+
+    // Send output buffers to be filled with encoded image.
+    while (mmal_wrapper_buffer_get_empty(portOut, &out, 0) == MMAL_SUCCESS) {
+      out->data = outBuf;


This sounds like it is going to be very bad if portOut->buffer_num > 1 as you'll use the same pointer for all the buffers.

I think I've been lucky so far and it's only ever given me a single buffer!
For this example, would it be better to use MMAL_WRAPPER_FLAG_PAYLOAD_ALLOCATE on portOut and have mmal handle this?

popcornmix · 2016-10-27T16:09:58Z

I suggested the name for this, so I'm okay with it.
If one day we have a more complete set of openmax/mmal image/video encode/decode examples we can come up with a new naming scheme (perhaps partly directory and partly filename) and this can be renamed.

6by9 · 2016-10-27T16:29:22Z

I suggested the name for this, so I'm okay with it.

:-)
I'll add the porting of the IL examples to MMAL to my list of jobs - it's the sort of trivial task that can be done without too much concentration of an evening, unlike some of the others. When done I'll discuss with you over directory/naming.

luked99 · 2016-10-28T05:53:17Z

host_applications/linux/apps/hello_pi/hello_mmal_encode/mmal_encode.c

+  // Perform the encoding
+
+  outBuf = malloc(portOut->buffer_size);
+  outFile = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0644);


Is there a reason for using open() rather than fopen() ? Won't open() mean that the output is unbuffered?

luked99 · 2016-10-28T05:53:49Z

host_applications/linux/apps/hello_pi/hello_mmal_encode/mmal_encode.c

+  outBuf = malloc(portOut->buffer_size);
+  outFile = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0644);
+  if (outFile < 0) {
+    fprintf(stderr, "Failed to open file: %s\n", filename);


Would be nice to print strerror(errno) here as well, to help people work out why it didn't succeed (same comment elsewhere).

luked99 · 2016-10-28T05:54:43Z

host_applications/linux/apps/hello_pi/hello_mmal_encode/mmal_encode.c

+
+    // Send image to be encoded.
+    if (!sent && mmal_wrapper_buffer_get_empty(portIn, &in, 0) == MMAL_SUCCESS) {
+      fprintf(stdout, "- sending %u bytes to encoder\n", inBufSize);


Perhaps just printf() rather than fprintf(stdout,...) ?

luked99

A few minor comments.

luked99 · 2016-10-28T05:55:51Z

host_applications/linux/apps/hello_pi/hello_mmal_encode/mmal_encode.c

+    // Send image to be encoded.
+    if (!sent && mmal_wrapper_buffer_get_empty(portIn, &in, 0) == MMAL_SUCCESS) {
+      fprintf(stdout, "- sending %u bytes to encoder\n", inBufSize);
+      memcpy(in->data, inBuf, inBufSize);


Is this ending up copying all of the input stream? Is there any way to avoid that?

I could construct the test image directly in the in->data buffer.
I'm also assuming the size of this buffer is always going to be >= inBuffSize (which it does always seem to be), but not sure if this is guaranteed?

luked99 · 2016-10-28T05:57:19Z

host_applications/linux/apps/hello_pi/hello_mmal_encode/mmal_encode.c

+uint32_t BLUE = 0xff << 16;
+uint32_t ALPHA = 0xff << 24;
+
+VCOS_SEMAPHORE_T sem;


Could this be static?

luked99 · 2016-10-28T05:59:10Z

host_applications/linux/apps/hello_pi/hello_mmal_encode/mmal_encode.c

+{
+  MMAL_WRAPPER_T* mmal;
+  unsigned int inBufSize;
+  unsigned char* inBuf;


If you made inBuf a void* then you wouldn't need the ugly casting later on.

sconemad · 2016-10-28T10:00:47Z

Hi, thanks for all the comments. I've pushed a further commit which hopefully addresses these.

popcornmix · 2016-10-30T14:27:20Z

@6by9 @luked99 okay with this now?

luked99 · 2016-10-31T09:46:41Z

Would you mind squashing those two commits together into one? I think it makes it easier to review.

I haven't actually tried to run it yet, but it looks pretty convincing to me. But a few more comments (sorry!).

Is the indentation intended to be just two spaces? In other code in the same directory it's 3 spaces (and either 4, or a hard tab would be my choice, but that's a flame war too far :-) But consistent with the rest of the code perhaps?

Should this be renamed to something like hello_mmal_image_encode, in case anyone ever adds a video encode example?

hello_mmal_encode: Updates following PR comments hello_mmal_encode: Change indent to match other examples

6by9 · 2016-10-31T14:26:52Z

@luked99

Is the indentation intended to be just two spaces? In other code in the same directory it's 3 spaces (and either 4, or a hard tab would be my choice, but that's a flame war too far :-) But consistent with the rest of the code perhaps?

Consistent would be good and appears to have been done. As you know, 3 was the Broadcom standard hence why it was applied to these files.
Hard tabs: you've been doing too much kernel hacking ;-) You'll be limiting line length to 80 chars next....

Should this be renamed to something like hello_mmal_image_encode, in case anyone ever adds a video encode example?

I already asked that question of popcornmix #349 (review) - he'd suggested the name :-)
I will start looking at porting the IL examples to MMAL, at which point we can put this and others in a MMAL directory and come up with a consistent naming scheme.

popcornmix · 2016-10-31T15:59:23Z

Many thanks @sconemad
Hopefully this will be the start of a collection of mmal examples that will help other users.

See: https://www.raspberrypi.org/forums/viewtopic.php?f=43&t=138290#p1052932 firmware: logging: Fix issue when logging end is at 1G firmware: hello_pi: Add hello_mmal_encode example See: raspberrypi/userland#349 firmware: IL ISP: Update to support opaque input firmware: IL ISP: Fix for opaque EOS and excessive finalise calls firmware: khronos: Use low 256M allocations for bin allocations

- firmware: isp: Add isp and isp_ilc to standard Pi variant as a cheap resize - firmware: MMAL: Add parameters to configure source pattern - firmware: IL Camera: Minor tidy up in RGB output handling - firmware: Video_render: Support YV12 and NV21 (YVU formats - firmware: IL Source: Add support for YV12 output (YVU420PackedPlanar) - firmware: IL ISP: Major updates - firmware: mmal_ril: Relay buffer alignment from RIL to MMAL - firmware: ldconfig: Increase line buffer length from 80 to 100 - firmware: h264: Fix skipping of SPSX in header bytes See: https://discourse.osmc.tv/t/video-does-not-work-with-hardware-acceleartion/6629/11 - firmware: display: Allow display blanking to affect DPI output See: #672 - firmware: Video_decode: Check licenced codecs at component create - firmware: dispmanx: Report transform or display as the display_rotate variable See: raspberrypi/userland#348 - firmware: arm_loader: Don't lose force_turbo when initial_turbo completes See: #667 - firmware: mmal: improvements to mmal_queue code - firmware: arm_dt: Silence system-supplied dtparams - firmware: vc_image: Remove obsolete processor support using _VC_VERSION - firmware: vc_image: Include colourspace in RGB to YUV conversions - firmware: OV5647/IMX219: Shutdown lines in reverse order to opening See: https://www.raspberrypi.org/forums/viewtopic.php?f=43&t=138290#p1052932 - firmware: logging: Fix issue when logging end is at 1G - firmware: hello_pi: Add hello_mmal_encode example See: raspberrypi/userland#349 - firmware: IL ISP: Update to support opaque input - firmware: IL ISP: Fix for opaque EOS and excessive finalise calls - firmware: khronos: Use low 256M allocations for bin allocations - firmware: logging: Fix issue when logging end is at 1G part 2 - firmware: arm_loader: Ensure mbox failure paths return failure - firmware: arm_loader: Add options for setting shared buffers from kernel driver - firmware: vcdbg: Use dma driver to access gpu memory - firmware: dispmanx: Support framebuffer_aspect=-1 for ignore aspect ratio See: #638 - firmware: Raspistill: Only fail setting restart int if not 0 - firmware: RaspiVid: -sg option could try opening null file - firmware: RaspiVid: Segment PTS file too if set See: raspberrypi/userland#354 - firmware: RaspiVid: Add raw (YUV420, RGB or grayscale) video output See: raspberrypi/userland#342 - firmware: Add support for EGL images allocated by VCSM See: raspberrypi/userland#344 - firmware: egl_client: Avoid missing return value warning - firmware: debug_sym: Use pointer type for user address - firmware: Video_splitter: store and pass on frame rate information - firmware: TC358762: Fix broken backlight ratelimiting

See: https://www.raspberrypi.org/forums/viewtopic.php?f=43&t=138290#p1052932 firmware: logging: Fix issue when logging end is at 1G firmware: hello_pi: Add hello_mmal_encode example See: raspberrypi/userland#349 firmware: IL ISP: Update to support opaque input firmware: IL ISP: Fix for opaque EOS and excessive finalise calls firmware: khronos: Use low 256M allocations for bin allocations

6by9 reviewed Oct 27, 2016

View reviewed changes

luked99 reviewed Oct 28, 2016

View reviewed changes

hello_pi: Add hello_mmal_encode example

e9d501d

hello_mmal_encode: Updates following PR comments hello_mmal_encode: Change indent to match other examples

sconemad force-pushed the hello_mmal_encode branch from 0b50650 to e9d501d Compare October 31, 2016 10:38

popcornmix merged commit 100f8bb into raspberrypi:master Oct 31, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hello_pi: Add hello_mmal_encode example #349

hello_pi: Add hello_mmal_encode example #349

sconemad commented Oct 27, 2016

6by9 left a comment

6by9 Oct 27, 2016

6by9 Oct 27, 2016

sconemad Oct 28, 2016

6by9 Oct 27, 2016

6by9 Oct 27, 2016

sconemad Oct 28, 2016

popcornmix commented Oct 27, 2016

6by9 commented Oct 27, 2016

luked99 Oct 28, 2016

luked99 Oct 28, 2016

luked99 Oct 28, 2016

luked99 left a comment

luked99 Oct 28, 2016

sconemad Oct 28, 2016

luked99 Oct 28, 2016

luked99 Oct 28, 2016

sconemad commented Oct 28, 2016

popcornmix commented Oct 30, 2016

luked99 commented Oct 31, 2016 •

edited

Loading

6by9 commented Oct 31, 2016

popcornmix commented Oct 31, 2016


		fprintf(stdout, "Encoding test image %s\n", filename);

		if (mmal_wrapper_create(&mmal, MMAL_COMPONENT_DEFAULT_IMAGE_ENCODER)

hello_pi: Add hello_mmal_encode example #349

hello_pi: Add hello_mmal_encode example #349

Conversation

sconemad commented Oct 27, 2016

6by9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popcornmix commented Oct 27, 2016

6by9 commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luked99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sconemad commented Oct 28, 2016

popcornmix commented Oct 30, 2016

luked99 commented Oct 31, 2016 • edited Loading

6by9 commented Oct 31, 2016

popcornmix commented Oct 31, 2016

luked99 commented Oct 31, 2016 •

edited

Loading