[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

--- Comment #17 from Carlos Rafael Giani <dv at pseudoterminal.org> ---
> @@ +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? :)

Hm I think it should be reset in the stop() vfunc. However, "if we don't get
config from caps" refers to the properties config, and its ready parameter is
set to TRUE in the _init function. (Same applies to rawvideoparse.)


> ::: 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?

Is this really a good idea? This sounds to me like the type of "intelligent"
behavior that can negatively surprise people. I'm not totally against it, just
concerned that this would make the usage more confusing.


> @@ +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?

Oh. I remembered the new_caps behavior incorrectly then - I thought it takes
ownership over the caps.


> @@ +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

This I do not understand. This is the _passthrough_ mode. The element is not
supposed to touch the data at all in this mode. The role of the raw parsers is
to parse _raw_ data. TIMES segments with timestamps is not raw data.


> Why not the object lock?

Because from discussions in #gstreamer I got the impression that the object
lock is supposed to be used by actual elements, not by base classes. Also, I do
call some base class functions within locked regions.


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

No. This is completely new code.

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