Created attachment 248293 [details] [review]
port to 1.0

Comment on attachment 248293 [details] [review]
port to 1.0

Thanks for the port. Looks very good at first glance.

>+#include <gst/video/navigation.h>
>+#include <gst/video/colorbalance.h>
>+#include <gst/video/gstvideometa.h>
>+
>+#include <gst/video/video.h>

The gst/video/video.h include should be enough now in git master.
 

>+    GST_STATIC_CAPS ("video/x-raw, "
>         "framerate = (fraction) [ 0, MAX ], "
>-        "width = (int) [ 1, MAX ], " "height = (int) [ 1, MAX ]")
>+        "width = (int) [ 1, MAX ], " "height = (int) [ 1, MAX ]; ")

Why add the semicolon at the end?


> static GstVideoSinkClass *parent_class = NULL;

This is no longer used (replaced by gst_dfb_blah_parent_class from G_DEFINE_TYPE), is it?

 
>+static gboolean
>+gst_dfb_buffer_pool_set_config (GstBufferPool * pool, GstStructure * config)
>+{
>+  pixel_format = gst_dfbvideosink_get_format_from_caps (caps);
>+
>+  structure = gst_caps_get_structure (caps, 0);
>+  if (!gst_structure_get_int (structure, "width", &width) ||
>+      !gst_structure_get_int (structure, "height", &height)) {
>+    GST_WARNING_OBJECT (pool, "failed getting geometry from caps %"
>+        GST_PTR_FORMAT, caps);
>+    return FALSE;
>+  }

One would/could use gst_video_info_from_caps() here as well.

>+  size = pitch * height;

Not sure this is right for pixel formats with multiple planes? (I420, YV12, NV12?)


>+static GstFlowReturn
>+gst_dfb_buffer_pool_alloc_buffer (GstBufferPool * bpool,
>+    GstBuffer ** buffer, GstBufferPoolAcquireParams * params)
> {
>+  switch (format) {
>+    case GST_VIDEO_FORMAT_I420:
>+      offset[1] = pitch * meta->height;
>+      offset[2] = pitch * meta->height * 3 / 2;
>+      stride[0] = stride[1] = stride[2] = pitch;

Shouldn't stride for the chroma planes  be pitch/2 ?

>+      plane_size[0] = offset[1];
>+      plane_size[1] = plane_size[2] = pitch * meta->height / 2;
>+      max_size = plane_size[0] * 2;
>+      n_planes = 3;
>+      break;
>+    case GST_VIDEO_FORMAT_YV12:
>+      offset[1] = pitch * meta->height;
>+      offset[2] = offset[1] + pitch / 2 * meta->height / 2;
>+      stride[0] = pitch;
>+      stride[1] = stride[2] = pitch / 2;
>+
>+      plane_size[0] = offset[1];
>+      plane_size[1] = plane_size[2] = plane_size[0] / 4;
>+      max_size = plane_size[0] * 3 / 2;
>+      n_planes = 3;
>+      break;

In general the calculation for I420 and YV12 should be identical, only that the U+V planes are swapped.


>+  for (i = 0; i < n_planes; i++) {
>+    gst_buffer_append_memory (surface,
>+        gst_memory_new_wrapped (0, data, max_size, offset[i], plane_size[i],
>+            NULL, NULL));
>+  }

Don't think you *need* to put each plane into its own GstMemory block if it's contiguous?


>@@ -1591,13 +1760,21 @@ gst_dfbvideosink_show_frame (GstBaseSink * bsink, GstBuffer * buf)
>+      /* Write each line respecting subsurface pitch */
>+      for (plane_line = 0; line < result.h || plane_line < plane_h;
>+          line++, plane_line++) {
>+        /* We do clipping */
>+        memcpy (w_buf, (gchar *) src_frame.data[plane] +
>+            (plane_line * src_info.stride[plane]),
>+            MIN (src_info.stride[plane], stride[plane]));
>+        w_buf += stride[plane];

Shouldn't the length of the memcpy be a function of the width somehow (or asked differently: do we want to memcpy the rowpadding as well)?

Created attachment 253363 [details] [review]
port to 1.0 v2

Thank you for your review.
I've taken your suggestions and attached the new patch.
Please see it.

>> +#include <gst/video/navigation.h>
>> +#include <gst/video/colorbalance.h>
>> +#include <gst/video/gstvideometa.h>
>> +
>> +#include <gst/video/video.h>
>
> The gst/video/video.h include should be enough now in git master.
The redundant includes have been removed.

>> +    GST_STATIC_CAPS ("video/x-raw, "
>>         "framerate = (fraction) [ 0, MAX ], "
>> -        "width = (int) [ 1, MAX ], " "height = (int) [ 1, MAX ]")
>> +        "width = (int) [ 1, MAX ], " "height = (int) [ 1, MAX ]; ")
>
> Why add the semicolon at the end?
Sorry, I've fixed the typo.

>> static GstVideoSinkClass *parent_class = NULL;
>
> This is no longer used (replaced by gst_dfb_blah_parent_class from
> G_DEFINE_TYPE), is it?
This has been replaced with the following modification.
It is copied from other plugins.

-static GstVideoSinkClass *parent_class = NULL;
+#define gst_dfbvideosink_parent_class parent_class

>> +static gboolean
>> +gst_dfb_buffer_pool_set_config (GstBufferPool * pool, GstStructure *
config)
>> +{
>> +  pixel_format = gst_dfbvideosink_get_format_from_caps (caps);
>> +
>> +  structure = gst_caps_get_structure (caps, 0);
>> +  if (!gst_structure_get_int (structure, "width", &width) ||
>> +      !gst_structure_get_int (structure, "height", &height)) {
>> +    GST_WARNING_OBJECT (pool, "failed getting geometry from caps %"
>> +        GST_PTR_FORMAT, caps);
>> +    return FALSE;
>> +  }
>
> One would/could use gst_video_info_from_caps() here as well.
width and height have been changed according to the value obtained by
gst_video_info_from_caps().
However pixel_format can't be replaced with the GStreamer video library
because it will contain the DirectFB color definition.

>> +  size = pitch * height;
>
> Not sure this is right for pixel formats with multiple planes? (I420,
YV12,
> NV12?)
Exactly. I've added the following processing in order to calculate
the planar formats sizes.

-  size = pitch * height;
+  switch (GST_VIDEO_INFO_FORMAT(&info)) {
+    case GST_VIDEO_FORMAT_I420:
+    case GST_VIDEO_FORMAT_YV12:
+    case GST_VIDEO_FORMAT_NV12:
+      size = pitch * height * 3 / 2;
+      break;
+    default:
+      size = pitch * height;
+      break;
+  }

> In general the calculation for I420 and YV12 should be identical, only
that the
> U+V planes are swapped.
I mistakenly took I420 for another color format.
The calculation for I420 has been changed to be the same as YV12.

>> +  for (i = 0; i < n_planes; i++) {
>> +    gst_buffer_append_memory (surface,
>> +        gst_memory_new_wrapped (0, data, max_size, offset[i],
plane_size[i],
>> +            NULL, NULL));
>> +  }
>
> Don't think you *need* to put each plane into its own GstMemory block
if it's
> contiguous?
I think the each plane should be added to the GstBuffer separately
even if it is contiguous.
Some upstream plugins deal with the planes as separated memory blocks
when the width and the stride are identical.

For instance, gst-omx could behave as I mentioned above.
It writes decoded images to the separated planes which is prepared
in sink plugin, using the hard-coded number of planes for each color
format.

gstomxvideodec.c
gst_omx_video_dec_fill_buffer()

>> +      /* Write each line respecting subsurface pitch */
>> +      for (plane_line = 0; line < result.h || plane_line < plane_h;
>> +          line++, plane_line++) {
>> +        /* We do clipping */
>> +        memcpy (w_buf, (gchar *) src_frame.data[plane] +
>> +            (plane_line * src_info.stride[plane]),
>> +            MIN (src_info.stride[plane], stride[plane]));
>> +        w_buf += stride[plane];
>
> Shouldn't the length of the memcpy be a function of the width somehow
(or asked
> differently: do we want to memcpy the rowpadding as well)?
The memcpy length has been calculated outside the loop beforehand.
The modification is as the following.

       plane_h = GST_VIDEO_FRAME_COMP_HEIGHT (&src_frame, plane);
+      size = MIN (src_info.stride[plane], stride[plane]);

       /* Write each line respecting subsurface pitch */
       for (plane_line = 0; line < result.h || plane_line < plane_h;
           line++, plane_line++) {
         /* We do clipping */
         memcpy (w_buf, (gchar *) src_frame.data[plane] +
-            (plane_line * src_info.stride[plane]),
-            MIN (src_info.stride[plane], stride[plane]));
+            (plane_line * src_info.stride[plane]), size);
         w_buf += stride[plane];

Thanks, will push this once the 1.1.4 development release is out.

Thanks, pushed. I made the "layer-mode" property an enum - wasn't sure though if a "none" enum value makes sense, or if it should just be "exclusive" or "administrative".


commit 8293594397c7687a9f5c1a3a39b77a7ac7900f59
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Thu Aug 29 15:46:14 2013 +0100

    dfbvideosink: make "layer-mode" property an enum
    
    https://bugzilla.gnome.org/show_bug.cgi?id=703520

commit f79448552ad4b429a09b30c9afcda997a61fb7d8
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Thu Aug 29 14:48:28 2013 +0100

    dfbvideosink: don't use deprecated GLib thread API
    
    https://bugzilla.gnome.org/show_bug.cgi?id=703520

commit 90020e21dfaf6039992ace18fe188a2145f4f9b1
Author: Kazunori Kobayashi <kkobayas@igel.co.jp>
Date:   Wed Jun 26 11:07:14 2013 +0900

    dfbvideosink: port to 1.0
    
    including the following supports and fixes:
    
    * Create DirectFB surfaces from GstBufferPool
    * Add NV12 pixel format support
    * Don't use the cursor in the exclusive mode
      - EnableCusor() can be only used when the administrative mode is set
        in DirectFB 1.6.0 and later.
    * Support multiple plane rendering for planar color formats
      - This accommodates the chroma plane offsets of the framebuffer
        in planar formats.
    * Invoke SetConfiguration regardless of video mode setting in setcaps()
      - SetConfiguration() method should be invoked regardless of
        the result of gst_dfbvideosink_get_best_vmode(), since the two are
        unrelated.
    * Disable DirectFB signal handler
      - "--dfb:no-sighandler" option is passed to DirectFBInit().
        This prevents DirectFB from trying to kill the process and allows
        GStreamer's termination sequence to proceed normally.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=703520