[Bug 767011] rawparse: new rawaudioparse and rawvideoparse element which deprecate audioparse, unalignedaudioparse, and videoparse

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=767011

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #330221|none                        |needs-work
             status|                            |

--- Comment #16 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 330221:
 --> (https://bugzilla.gnome.org/review?bug=767011&attachment=330221)

Unit tests \o/ I only shortly looked over it now and wrote down everything that
I noticed

::: gst/rawparse/gstrawaudioparse.c
@@ +864,3 @@
+        channel_mask, config->channel_positions);
+  } else
+    return TRUE;

Curly braces

@@ +1039,3 @@
+  gst_caps_unref (caps);
+  if (ret)
+    config->ready = TRUE;

You only set this to ready here, nowhere else? What if we don't get config from
caps? :)

::: gst/rawparse/gstrawbaseparse.c
@@ +126,3 @@
+
+
+#define DEFAULT_USE_SINK_CAPS  TRUE     /* default is TRUE to facilitate
autoplugging */

Subclasses should maybe set it automatically to FALSE if any of the other
properties is set?

@@ +382,3 @@
+    /* Push caps outside of the lock */
+    gst_pad_push_event (GST_BASE_PARSE_SRC_PAD (raw_base_parse),
+        gst_event_new_caps (new_src_caps)

The caps should probably be unreffed?

@@ +460,3 @@
+    }
+
+    new_caps_event = gst_event_new_caps (new_src_caps);

And unref the caps here?

@@ +527,3 @@
+    frame->out_buffer = processed_data;
+  } else
+    frame->out_buffer = NULL;

Curly braces please

@@ +548,3 @@
+  if (G_UNLIKELY (new_caps_event != NULL))
+    gst_pad_push_event (GST_BASE_PARSE_SRC_PAD (raw_base_parse),
+        new_caps_event);

There are various return-paths between this and creating the caps event, where
the event is not unreffed

@@ +672,3 @@
+       * assume the buffers are timestamped properly and
+       * this rawbaseparse element does not actually have
+       * to do anything, so put it in passthrough mode.

It has to make sure the data is correctly chunked at least, or not? Upstream
could know timestamps but not frame sizes

@@ +728,3 @@
+      if (new_src_caps)
+        gst_pad_push_event (GST_BASE_PARSE_SRC_PAD (parse),
+            gst_event_new_caps (new_src_caps));

Also caps leaked

@@ +784,3 @@
+  /* must be called with lock */
+  g_assert (raw_base_parse != NULL);
+  raw_base_parse->src_caps_set = FALSE;

This, and probably also some other things, should probably be reset in
GstBaseParse::stop()

::: gst/rawparse/gstrawbaseparse.h
@@ +23,3 @@
+#include <gst/gst.h>
+#include <gst/base/gstadapter.h>
+#include <gst/base/gstbaseparse.h>

#include <gst/base/base.h>

@@ +43,3 @@
+
+#define GST_RAW_BASE_PARSE_CONFIG_MUTEX_LOCK(obj)  
g_mutex_lock(&(((GstRawBaseParse *)(obj))->config_mutex))
+#define GST_RAW_BASE_PARSE_CONFIG_MUTEX_UNLOCK(obj)
g_mutex_unlock(&(((GstRawBaseParse *)(obj))->config_mutex))

Why not the object lock?

::: gst/rawparse/gstrawvideoparse.c
@@ +1,2 @@
+/* GStreamer
+ * Copyright (C) <2016> Carlos Rafael Giani <dv at pseudoterminal dot org>

In general, you probably want to copy over the old copyrights of the files. I
think you based some code on the old one, right?

@@ +232,3 @@
+          "Width",
+          "Width of frames in raw stream",
+          0, INT_MAX, DEFAULT_WIDTH, G_PARAM_READWRITE |
G_PARAM_STATIC_STRINGS)

G_MAXINT, etc

@@ +346,3 @@
+gst_raw_video_parse_dispose (GObject * object)
+{
+  //GstRawVideoParse *raw_video_parse = GST_RAW_VIDEO_PARSE(object);

No // comments

@@ +796,3 @@
+        GST_VIDEO_INFO_PAR_D (&(config_ptr->info));
+    config_ptr->framerate[0] = GST_VIDEO_INFO_FPS_N (&(config_ptr->info));
+    config_ptr->framerate[1] = GST_VIDEO_INFO_FPS_D (&(config_ptr->info));

Why an array? We everywhere else use _n and _d :) Seems inconsistent

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.