Created attachment 328712 [details] [review]
New rawaudioparse element

Forgot to add:

I opted for a rewrite instead of porting the audioparse code since baseparse makes the rawparse class almost completely redundant and the rest would be a mess. Also, this is an opportunity to introduce a more systematic naming scheme. "rawaudioparse" is much clearer than "audioparse". It also means that a future "rawvideoparse" could be introduced as well.

Review of attachment 328712 [details] [review]:

Finally, someone ports our "most simple format" parser to baseparse! Thank you!

gstrawaudiocommon.h  is common with what ?

::: gst/rawparse/gstrawaudioparse.c
@@ +706,3 @@
+        );
+
+    raw_audio_parse->src_caps_set = TRUE;

Why set this outside the lock?

@@ +822,3 @@
+    GST_ERROR_OBJECT (raw_audio_parse,
+        "only %" G_GSIZE_FORMAT " bytes available, which is not enough", size);
+    flow_ret = GST_FLOW_ERROR;

This should never happen, you should just do a g_assert(). FYI, when returning GST_FLOW_ERROR, you MUST post a message on the bus using GST_ELEMENT_ERROR(), but in this case, g_assert() is enough.

@@ +923,3 @@
+       * bytes from a previous stream no longer apply.
+       * So, assume that no sink caps have ever been seen,
+       * and that the source pad caps have never been set. */

Is this true? I don't think stream start means that.

@@ +941,3 @@
+      GST_OBJECT_LOCK (raw_audio_parse);
+      gst_adapter_clear (raw_audio_parse->adapter);
+      raw_audio_parse->src_caps_set = FALSE;

flushes don't change the caps.

::: gst/rawparse/gstrawaudioparse.h
@@ +125,3 @@
+   * samples inside.
+   * The adapter is cleared during FLUSH_STOP and STREAM_START events. */
+  GstAdapter *adapter;

Why not use the adapter that's already in BaseParse? Setting the min_frame_size means you should only get whole "frames".

> Is this true? I don't think stream start means that.

Then what does stream start mean, if not a start of a completely new stream?

And:

> Why not use the adapter that's already in BaseParse? Setting the min_frame_size means you should only get whole "frames".

Is this true? I understood min_frame_size as the minimum size of a buffer, as indicated by this part of the docs: "A buffer of (at least) min_frame_size bytes is passed to subclass with handle_frame." On the other hand, the docs also mention: "Inform base class how big data chunks should be retrieved. This is done with gst_base_parse_set_min_frame_size() function." I suppose "how big data chunks" refers to multiples of min_frame_size?

Review of attachment 328712 [details] [review]:

::: gst/rawparse/gstrawaudioparse.c
@@ +746,3 @@
+      GST_ERROR_OBJECT (parse,
+          "trying to push data even though current config is not ready");
+      flow_ret = GST_FLOW_ERROR;

In which case could this happen? If the src caps were rejected? Then it's GST_FLOW_NOT_NEGOTIATED? Otherwise, do GST_ELEMENT_ERROR() before returning GST_FLOW_ERROR.

@@ +866,3 @@
+
+  if (config->bpf == 0) {
+    GST_ERROR_OBJECT (parse, "trying to convert even though config->bpf is 0");

In which case can the ->bpf be 0? Shouldn't it always be valid either from the sink caps or from the properties?

(In reply to Carlos Rafael Giani from comment #4)
> Then what does stream start mean, if not a start of a completely new stream?

It means a new "logical" stream, I don't think it requires the caps to change or be re-sent.

(In reply to Carlos Rafael Giani from comment #5)
> Is this true? I understood min_frame_size as the minimum size of a buffer,
> as indicated by this part of the docs: "A buffer of (at least)
> min_frame_size bytes is passed to subclass with handle_frame." On the other
> hand, the docs also mention: "Inform base class how big data chunks should
> be retrieved. This is done with gst_base_parse_set_min_frame_size()
> function." I suppose "how big data chunks" refers to multiples of
> min_frame_size?


Min size is indeed just a minimum, but if the base class gives you something that is more than the multiple of the "bpf, you can call _finish_frame() with a multiple of bpf and the following bytes will be given to you again in the next ->handle_frame(), the adapter in the base class is there exactly for the kind of stuff your doing, because almost all parsers have something like that.

Created attachment 328929 [details] [review]
New rawaudioparse element, v2

Updated patch which addresses the aforementioned issues, adds new checks for corner cases (such as when upstream pushes a TIME segment and properties config is active), and adds unit tests.

There is one corner case that I haven't been able to figure out yet. Take this pipeline:

  gst-launch-1.0 fakesrc filltype=5 sizetype=fixed sizemax=4096 ! "audio/x-raw,rate=44100,channels=2,format=S16LE,layout=interleaved" ! rawaudioparse ! fakesink sync=true

In this case, rawaudioparse receives stream-start , and then immediately afterwards data. This is because fakesrc is operating in pull mode. If can-activate-pull=false is added to fakesrc, it works. The question is how to catch this properly, or if it even makes sense to do so.

Also, if I replace rawaudioparse with audioconvert for example, it works. I guess this is because audioconvert operates in push mode.

That case seems to also fail with the old "audioparse". The way I understand pull mode, the parser has to be able to just pull data without receiving segment or caps. I expected BaseSrc to push a segment event, but it seems to not do that in pull mode.

In pull mode (if you pull from upstream) you never get (and if you do it's a bug) events or anything pushed from upstream.

Alright, then I'll not consider it a bug, and document it instead.

However, please hold back on committing this for now. I was able to begin the rawvideoparse development sooner than anticipated, and I'm extracting common rawaudio/rawvideo functionality to a rawbaseparse base class (which unlike the old rawparse base class is written on top of baseparse and has the config switching functionality that rawaudioparse already has). I'll update this in a few days.

Created attachment 330221 [details] [review]
Patch with new audio and video parser elements

Thanks to many interferences, it took a little more than a few days :) This adds the rawaudioparse and rawvideoparse elements. They are both based on the new GstRawBaseParse class, which in turn is based on GstBaseParse. Tests for rawaudioparse and rawvideoparse are also added.

The old videoparse and audioparse elements are not needed anymore, and can be removed in a followup commit.

(In reply to Carlos Rafael Giani from comment #13)

> The old videoparse and audioparse elements are not needed anymore, and can
> be removed in a followup commit.

Would be good to register additional element factories for them though, for backwards compatibility for a while (and let them print deprecation warnings when used).

(In reply to Sebastian Dröge (slomo) from comment #14)

> Would be good to register additional element factories for them though, for
> backwards compatibility for a while (and let them print deprecation warnings
> when used).

Alternatively, we could keep the old elements in 1.10, with deprecation warnings, and remove them in 1.12.

Review of attachment 330221 [details] [review]:

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

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

(In reply to Carlos Rafael Giani from comment #17)

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

Upstream could for example be adaptivedemux, which sends a TIME segment and the initial PTS for each fragment. However the data that comes is not chunked (it's just souphttpsrc in the end).

Then perhaps we could introduce video/x-unaligned-raw , just like with audio. The passthrough mode is useful if for some reason a raw parser is autoplugged even though the incoming data is already timestamped and chunked. This can happen for example with rawaudioparse and mulaw content.

It should never be autoplugged for audio/x-raw

"The initial PTS for each fragment" meaning ... what? Several buffers will have the same PTS from the corresponding fragment? And this means that the raw parsers should (re)timestamp and set durations even with TIME segments? That is, never enter passthrough mode?

The first one will have a PTS, the others will have NONE. The parser (and/or other elements) are supposed to take the timestamps they get and interpolate any missing timestamps.

So this essentially means that I just have to dismantle the passthrough code, since this is what baseparse does anyway. The rawparsers already set the duration, and this is all that's needed. I can add an extra check to see if the duration is already set, and only if it isn't, I set my own.

Sounds good :)

Created attachment 330426 [details] [review]
Patch with new audio and video parser elements, v2

Updated patch with the aforementioned issues fixed. Passthrough mode was removed. No special handling added, since there was little gain in it, and just complicated the code.

I think my main concern here is now that we would always "autoplug" these elements for audio/x-raw and video/x-raw, which might not be ideal. Maybe there should be one variant that only registers {audio,video}/x-raw-unaligned and has PRIMARY rank, and the other variant has all caps and NONE rank.

I thought these caps won't have any autoplugging?

But we could keep the unalignedaudioparse element and set the default use-sink-caps property values to FALSE. Also what about the idea of a video/x-unaligned-raw caps and a new unalignedvideoparse element?

That seems useful, yes. Having unaligned elements around the new parsers that take the x-unaligned caps, are autoplugged and use the caps by default. And having the normal ones with NONE rank.

Created attachment 330608 [details] [review]
Patch with new audio and video parser elements, v3

Set rawaudioparse & rawvideoparse ranks to NONE, and added an unalignedvideoparse element. unalignedaudioparse now uses rawaudioparse instead of audioparse.

I'd like to propose this patch for the 1.9.2 release.

We should get this in for 1.10, yes

Review of attachment 330608 [details] [review]:

rawvideoparse is still missing the frame-size property (there could be more padding at the very end of a frame), otherwise this looks good to go to me.

Main reason why we can't remove the old parsers at this point is the property name changes, right? Apart from that it should be compatible?

::: gst/rawparse/gstrawaudioparse.h
@@ +89,3 @@
+   * for reordering channel data in outgoing buffers if necessary.
+   */
+  GstAudioChannelPosition *valid_channel_positions;

The two arrays here can just be statically allocated [64] arrays. Less memory to keep track of

::: gst/rawparse/gstrawvideoparse.c
@@ +940,3 @@
+   * from here */
+  while (TRUE) {
+    GstVideoMeta *videometa = gst_buffer_get_video_meta (out_data);

while ((videometa = gst_buffer_get_video_meta (out_data)) { ... }

might be more beautiful... but not so important :)

frame-stride is the replacement for frame-size. It accomplishes exactly what you described (handling padding at the end of a frame).

Oh, and forgot to add: I thought since the old parsers are in -bad , API changes are OK? Including the removal of plugins? So why do we have to keep the old ones now?

We should mark the existing elements as deprecated and let them print warnings and everything, and *ideally* make them just wrappers with the same interface around the new elements for a release or two. Otherwise code out there is just falling apart without warning.

Okay. So should this deprecation / wrapping be done in a separate followup commit, or in this same one? What would be preferred?

Make it another commit

Created attachment 332096 [details] [review]
330608: Patch with new audio and video parser elements, v4

Created attachment 332098 [details] [review]
Patch with new audio and video parser elements, v4

Typo

Created attachment 332106 [details] [review]
Patch with new audio and video parser elements, v5

Plane offset / stride values were not being tested for, and in fact were swapped internally (planet-offset property returned stride values and vice versa). Fixed & added test for these values.

Created attachment 332110 [details] [review]
Old parser wrapper patch

Here is a patch to wrap the new parsers in the old parser interfaces, together with deprecation notes.

Created attachment 332111 [details] [review]
Update rawparser documentation

And here are documentation updates

commit efa5b219e65ab3614270630eaf1163b3dc6f2dad
Author: Carlos Rafael Giani <dv@pseudoterminal.org>
Date:   Mon Jul 25 17:55:24 2016 +0200

    docs: Update rawparser documentation
    
    Signed-off-by: Carlos Rafael Giani <dv@pseudoterminal.org>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767011

commit 403499a0c015920d367ba62531d515cfced1c3f3
Author: Carlos Rafael Giani <dv@pseudoterminal.org>
Date:   Mon Jul 25 17:54:09 2016 +0200

    rawparse: Remove old parser code and wrap new parsers in old elements
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767011

commit d555f34562d8e477383df4d9f2b851db7de2af51
Author: Carlos Rafael Giani <dv@pseudoterminal.org>
Date:   Mon Jul 25 13:45:40 2016 +0200

    rawparse: Add new raw audio and video parser elements
    
    The new rawaudioparse and rawvideoparse elements are based on GstBaseParse
    and completely replace audioparse and videoparse
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767011