Created attachment 366379 [details] [review]
GStreamer based writeback module

Created attachment 366396 [details] [review]
GStreamer based writeback module

minor changes.

Review of attachment 366396 [details] [review]:

Very nice overall :). I have some comments on it, but nothing major IMHO (except towards being conservative wrt preserving the taglib module).

::: configure.ac
@@ +1888,3 @@
 Writeback Formats:
 
+	Audio files using Gstreamer:            $have_gstreamer_audio_writeback

Nit, let's call it GStreamer (uppercase S), that's the preferred use of capitals AFAICS.

::: meson.build
@@ +408,3 @@
   summary += [
     '\nWriteback Formats:',
+    '    Audio files using Gsteamer:             ' + (gstreamer.found() and gstreamer_tag.found() and gstreamer_audio.found()).to_string(),

Nit, missing 'r' and capital 'S' in GStreamer.

::: src/tracker-writeback/tracker-writeback-gstreamer.c
@@ +30,3 @@
+
+#include <libtracker-sparql/tracker-ontologies.h>
+#include <libtracker-sparql/tracker-ontologies.h>

Repeated include :)

@@ +123,3 @@
+		"audio/x-ac3",
+		"audio/ogg",
+		"audio/x-ogg",

Hmm, one thing that concerns me a bit if we're to replace taglib entirely is the loss of supported formats. How about not removing the taglib module yet, but removing all these mimetypes from that module so both may coexist? We get the gstreamer module handling the most common formats, and the taglib module to back it up for more esoteric ones.

Being taglib an optional dependency, I'm not too concerned if the dependency has to stay, it's certainly not the craziest one :).

@@ +131,3 @@
+
+static gboolean
+link_named_pad (GstPad *srcpad, GstElement *element, const gchar *sinkpadname)

Code style, each argument on its own line. This happens in a few other functions below

@@ +164,3 @@
+	gst_bin_add (GST_BIN (pipeline), tagger);
+	if (!link_named_pad (srcpad, tagger, "sink"))
+		return NULL;

should this path additionally do gst_bin_remove()? The same question applies to *_tagger().

@@ +216,3 @@
+	gst_bin_add_many (GST_BIN (pipeline), parser, tagger, mux, NULL);
+	if (!link_named_pad (srcpad, parser, "sink"))
+		goto error;

However here (and from now on in this function), I think the "goto error" paths are more harmful than just a leak. Unref'ing those GstElements below the pipeline's feet (which now owns those) sounds like something gstreamer wouldn't like.

@@ +248,3 @@
+
+	gst_bin_add (GST_BIN (pipeline), mux);
+	if (!link_named_pad (srcpad, mux, "audio_%u"))

%u? :)

@@ +396,3 @@
+
+static gboolean
+make_temp (const gchar *uri, gchar **uri_final, GOutputStream **stream, GError **io_error)

This function can simply go away :). src/tracker-writeback/tracker-writeback-file.c already does a temporary copy using g_mkstemp_full() and gives that path to writeback modules.

@@ +753,3 @@
+
+		/* Maybe artwork is actually an nfo:Image and query its url first?
+		 * Right now assuming absolute path for image in row[3] */

This is unhandled yet on the extraction side. But effectively artwork is an nfo:Image, presumably it should be a nfo:FileDataObject too and have a nie:url.

I think it is fine if you comment this out and add a FIXME, or make it query the nie:url of the nmm:artwork already. It should be ineffective still anyway...

@@ +826,3 @@
+	uri = g_file_get_uri (file);
+	writeback_gstreamer_save (element, uri, error);
+	g_free (uri);

I suggest just making writeback_gstreamer_save() take a GFile argument instead of an URI.

@@ +868,3 @@
+	}
+
+	g_object_unref (cursor);

Use g_clear_object(), this will result in a warning if we get a NULL cursor here.

(In reply to Carlos Garnacho from comment #3)
> Review of attachment 366396 [details] [review] [review]:

Thanks for the review. I know you've been busy.

> ::: configure.ac
> @@ +1888,3 @@
>  Writeback Formats:
>  
> +	Audio files using Gstreamer:            $have_gstreamer_audio_writeback
> 
> Nit, let's call it GStreamer (uppercase S), that's the preferred use of
> capitals AFAICS.
> 
> ::: meson.build
> @@ +408,3 @@
>    summary += [
>      '\nWriteback Formats:',
> +    '    Audio files using Gsteamer:             ' + (gstreamer.found() and
> gstreamer_tag.found() and gstreamer_audio.found()).to_string(),
> 
> Nit, missing 'r' and capital 'S' in GStreamer.

Whoops! spelling errors. :P

> ::: src/tracker-writeback/tracker-writeback-gstreamer.c
> @@ +30,3 @@
> +
> +#include <libtracker-sparql/tracker-ontologies.h>
> +#include <libtracker-sparql/tracker-ontologies.h>
> 
> Repeated include :)
> 
> @@ +123,3 @@
> +		"audio/x-ac3",
> +		"audio/ogg",
> +		"audio/x-ogg",
> 
> Hmm, one thing that concerns me a bit if we're to replace taglib entirely is
> the loss of supported formats. How about not removing the taglib module yet,
> but removing all these mimetypes from that module so both may coexist? We
> get the gstreamer module handling the most common formats, and the taglib
> module to back it up for more esoteric ones.
> 
> Being taglib an optional dependency, I'm not too concerned if the dependency
> has to stay, it's certainly not the craziest one :).

Sure. My only concern, was that I was not sure how tracker-writeback would
behave with two audio writeback modules present, as in the order of calling
and premature tagging if either was found. But it loads both modules and tags
fine on my setup.

> @@ +131,3 @@
> +
> +static gboolean
> +link_named_pad (GstPad *srcpad, GstElement *element, const gchar
> *sinkpadname)
> 
> Code style, each argument on its own line. This happens in a few other
> functions below

Got it.

> @@ +164,3 @@
> +	gst_bin_add (GST_BIN (pipeline), tagger);
> +	if (!link_named_pad (srcpad, tagger, "sink"))
> +		return NULL;
> 
> should this path additionally do gst_bin_remove()? The same question applies
> to *_tagger().

Quoting from GstPipeline's documentation [0], "... when you are done with the pipeline, use gst_object_unref() to free its resources including all added GstElement objects (if not otherwise referenced)." So we do not need to remove each element added and unref them separately, since pipeline itself will handle all this for us.

But, at the same time...
 
> @@ +216,3 @@
> +	gst_bin_add_many (GST_BIN (pipeline), parser, tagger, mux, NULL);
> +	if (!link_named_pad (srcpad, parser, "sink"))
> +		goto error;
> 
> However here (and from now on in this function), I think the "goto error"
> paths are more harmful than just a leak. Unref'ing those GstElements below
> the pipeline's feet (which now owns those) sounds like something gstreamer
> wouldn't like.

.. this would definitely not be okay. Unreferencing elements before removing
them from the pipeline is not a valid move and as you said gstreamer would
not like it. I should correct this.

> @@ +248,3 @@
> +
> +	gst_bin_add (GST_BIN (pipeline), mux);
> +	if (!link_named_pad (srcpad, mux, "audio_%u"))
> 
> %u? :)

`audio_%u` is just the strange pad-name used to connect to the audio stream
of an MPEG4 file. See mp4mux documentation [1].

> @@ +396,3 @@
> +
> +static gboolean
> +make_temp (const gchar *uri, gchar **uri_final, GOutputStream **stream,
> GError **io_error)
> 
> This function can simply go away :).
> src/tracker-writeback/tracker-writeback-file.c already does a temporary copy
> using g_mkstemp_full() and gives that path to writeback modules.

I knew of this function earlier, but dismissed it because I needed an output
stream as well as a temporary file. I was not aware that the modules got
a temporary file themselves, in which case, we can get rid of this.

> @@ +753,3 @@
> +
> +		/* Maybe artwork is actually an nfo:Image and query its url first?
> +		 * Right now assuming absolute path for image in row[3] */
> 
> This is unhandled yet on the extraction side. But effectively artwork is an
> nfo:Image, presumably it should be a nfo:FileDataObject too and have a
> nie:url.
> 
> I think it is fine if you comment this out and add a FIXME, or make it query
> the nie:url of the nmm:artwork already. It should be ineffective still
> anyway...

Going to make it query for artwork url already. Why deviate from the standard.

> @@ +826,3 @@
> +	uri = g_file_get_uri (file);
> +	writeback_gstreamer_save (element, uri, error);
> +	g_free (uri);
> 
> I suggest just making writeback_gstreamer_save() take a GFile argument
> instead of an URI.

Alright, this makes sense considering its a temporary file we are working with.

> @@ +868,3 @@
> +	}
> +
> +	g_object_unref (cursor);
> 
> Use g_clear_object(), this will result in a warning if we get a NULL cursor
> here.

Sure.

I'll put together another patch, with the corrections, asap.

[0] https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstPipeline.html
[1] https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-good-plugins/html/gst-plugins-good-plugins-mp4mux.html

Created attachment 367595 [details] [review]
Add GStreamer based writeback module

Made corrections and changes

GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new enhancement request ticket at
  https://gitlab.gnome.org/GNOME/tracker/-/issues/

Thank you for your understanding and your help.