[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

Kazunori Kobayashi <kkobayas> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #248293|0                           |1
        is obsolete|                            |

--- Comment #3 from Kazunori Kobayashi <kkobayas at igel.co.jp> 2013-08-28 10:30:09 UTC ---
Created an attachment (id=253363)
 View: https://bugzilla.gnome.org/attachment.cgi?id=253363
 Review: https://bugzilla.gnome.org/review?bug=703520&attachment=253363

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];

-- 
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.