[Bug 703520] dfbvideosink: port to 1.0
GStreamer (bugzilla.gnome.org)
bugzilla at gnome.org
Sun Aug 18 06:49:57 PDT 2013
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.
More information about the gstreamer-bugs
mailing list