[Bug 703520] dfbvideosink: port to 1.0

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=703520
  GStreamer | gst-plugins-bad | git

Tim-Philipp Müller <t.i.m> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #248293|none                        |reviewed
             status|                            |

--- Comment #2 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2013-08-18 13:49:50 UTC ---
(From update of attachment 248293)
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)?

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.