After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 680424 - Port to gstreamer 1.0
Port to gstreamer 1.0
Status: RESOLVED FIXED
Product: tracker
Classification: Core
Component: General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: tracker-general
Jamie McCracken
3.6.1
Depends on:
Blocks: 679412
 
 
Reported: 2012-07-23 04:03 UTC by Matthias Clasen
Modified: 2012-10-24 15:23 UTC
See Also:
GNOME target: ---
GNOME version: 3.5/3.6


Attachments
Port-discoverer-backend-to-GStreamer-1.0 (4.66 KB, patch)
2012-08-21 20:33 UTC, Javier Jardón (IRC: jjardon)
reviewed Details | Review
Port-discoverer-backend-to-GStreamer-1.0.v2 (4.79 KB, patch)
2012-08-22 19:01 UTC, Javier Jardón (IRC: jjardon)
reviewed Details | Review
Port-discoverer-backend-to-GStreamer-1.0.v3 (5.79 KB, patch)
2012-09-16 20:43 UTC, Javier Jardón (IRC: jjardon)
needs-work Details | Review
Port-discoverer-backend-to-GStreamer-1.0.v4 (5.88 KB, patch)
2012-09-17 03:54 UTC, Javier Jardón (IRC: jjardon)
reviewed Details | Review
Port-discoverer-backend-to-GStreamer-1.0.v5 (5.92 KB, patch)
2012-09-18 16:58 UTC, Javier Jardón (IRC: jjardon)
committed Details | Review

Description Matthias Clasen 2012-07-23 04:03:49 UTC
See https://live.gnome.org/GnomeGoals/PortToGstreamer1
Comment 1 Javier Jardón (IRC: jjardon) 2012-08-21 20:33:13 UTC
Created attachment 222075 [details] [review]
Port-discoverer-backend-to-GStreamer-1.0

This patch only ports the dicoverer backend (the one by default)
Comment 2 Tim-Philipp Müller 2012-08-21 21:17:50 UTC
Comment on attachment 222075 [details] [review]
Port-discoverer-backend-to-GStreamer-1.0

Cool, thanks for working on this. Some quick drive-by comments:

>--- a/src/tracker-extract/tracker-extract-gstreamer.c
>+++ b/src/tracker-extract/tracker-extract-gstreamer.c
>@@ -407,24 +407,23 @@ get_embedded_cue_sheet_data (GstTagList *tag_list)
> static gboolean
> get_embedded_media_art (MetadataExtractor *extractor)
> {
>-	const GValue *value;
>+	GstSample *sample;
> 	guint lindex;
> 
> 	lindex = 0;
> 
> 	do {
>-		value = gst_tag_list_get_value_index (extractor->tagcache, GST_TAG_IMAGE, lindex);
>+		gst_tag_list_get_sample_index (extractor->tagcache, GST_TAG_IMAGE, lindex, &sample);

Should probably do

  if (!gst_..get_sample_index (...))
    break;

sample will be left uninitialized otherwise, _get_sample_index() won't set it to NULL in case of failure.

 
>-		if (value) {
>+		if (sample) {
> 			GstBuffer *buffer;
> 			GstCaps *caps;
> 			GstStructure *caps_struct;
> 			gint type;
> 
>-			buffer = gst_value_get_buffer (value);
>-			caps = gst_buffer_get_caps (buffer);
>-			caps_struct = gst_caps_get_structure (buffer->caps, 0);
>-
>+			buffer = gst_sample_get_buffer (sample);
>+			caps = gst_sample_get_caps (sample);
>+			caps_struct = gst_caps_get_structure (caps, 0);
> 			gst_structure_get_enum (caps_struct,
> 			                        "image-type",
> 			                        GST_TYPE_TAG_IMAGE_TYPE,
>@@ -432,33 +431,46 @@ get_embedded_media_art (MetadataExtractor *extractor)
> 
> 			if (type == GST_TAG_IMAGE_TYPE_FRONT_COVER ||
> 			    (type == GST_TAG_IMAGE_TYPE_UNDEFINED && extractor->media_art_buffer_size == 0)) {
>-				extractor->media_art_buffer = buffer->data;
>-				extractor->media_art_buffer_size = buffer->size;
>+				GstMapInfo info;
>+
>+				if (!gst_buffer_map (buffer, &info, GST_MAP_READ))
>+					return FALSE;
>+
>+				extractor->media_art_buffer = info.data;
>+				extractor->media_art_buffer_size = info.size;
> 				extractor->media_art_buffer_mime = gst_structure_get_name (caps_struct);
>-				gst_caps_unref (caps);
>+
>+				gst_buffer_unmap (buffer, &info);

Not sure it's really kosher to unmap the buffer, but store (and read) info.data for later. It's probably going to work fine in any sane real-world use case, especially if the taglist and hence the sample and hence the buffer are going to be kept around alongside it, just not 100% correct. (Wouldn't worry too much about it though, just saying).

> 
> 				return TRUE;
> 			}
> 
>-			gst_caps_unref (caps);
>-
> 			lindex++;

I think we need a gst_sample_unref (sample); here as well (and in the return cases above), since get_sample_index() returns a ref.

Most of the above comments apply to the code below as well.

 		}
>-	} while (value);
> 
>-	value = gst_tag_list_get_value_index (extractor->tagcache, GST_TAG_PREVIEW_IMAGE, lindex);
>+	} while (sample);
>+
>+	gst_tag_list_get_sample_index (extractor->tagcache, GST_TAG_IMAGE, lindex, &sample);
> 
>-	if (value) {
>+	if (sample) {
> 		GstBuffer *buffer;
>+		GstCaps *caps;
> 		GstStructure *caps_struct;
>+		GstMapInfo info;
> 
>-		buffer = gst_value_get_buffer (value);
>-		caps_struct = gst_caps_get_structure (buffer->caps, 0);
>+		buffer = gst_sample_get_buffer (sample);
>+		caps = gst_sample_get_caps (sample);
>+		caps_struct = gst_caps_get_structure (caps, 0);
>+
>+		if (!gst_buffer_map (buffer, &info, GST_MAP_READ))
>+			return FALSE;
> 
>-		extractor->media_art_buffer = buffer->data;
>-		extractor->media_art_buffer_size = buffer->size;
>+		extractor->media_art_buffer = info.data;
>+		extractor->media_art_buffer_size = info.size;
> 		extractor->media_art_buffer_mime = gst_structure_get_name (caps_struct);
> 
>+		gst_buffer_unmap (buffer, &info);
>+		gst_sample_unref (sample);
> 
> 		return TRUE;
> 	}
>@@ -1971,7 +1983,7 @@ tracker_extract_gstreamer (const gchar          *uri,
> 
> 	extractor = g_slice_new0 (MetadataExtractor);
> 	extractor->mime = type;
>-	extractor->tagcache = gst_tag_list_new ();
>+	extractor->tagcache = gst_tag_list_new_empty ();
> 	extractor->media_art_type = TRACKER_MEDIA_ART_NONE;
> 
> 	g_debug ("GStreamer backend in use:");
>-- 
>1.7.11.4
>
Comment 3 Javier Jardón (IRC: jjardon) 2012-08-22 19:01:03 UTC
Created attachment 222180 [details] [review]
Port-discoverer-backend-to-GStreamer-1.0.v2

Updated patch with your comments
Comment 4 Tim-Philipp Müller 2012-08-23 08:41:53 UTC
Comment on attachment 222180 [details] [review]
Port-discoverer-backend-to-GStreamer-1.0.v2

Looks better, thanks.

I think the coverart buffer may be leaked though, because now we never unmap the buffer, if I'm not mistaken. The easiest workaround for that would be to add the GstMapInfo struct into the MetadataExtractor structure, which seems private to the gstreamer extractor, and then make sure it gets unmapped in the appropriate place. Would probably need some ifdefs.
Comment 5 André Klapper 2012-09-13 13:09:03 UTC
Javier: Do you plan to update your patch based on comment 4?
Comment 6 Javier Jardón (IRC: jjardon) 2012-09-16 20:43:29 UTC
Created attachment 224461 [details] [review]
Port-discoverer-backend-to-GStreamer-1.0.v3

If I understand you comments correctly I think this is the correct patch
Comment 7 Tim-Philipp Müller 2012-09-16 20:56:04 UTC
Comment on attachment 224461 [details] [review]
Port-discoverer-backend-to-GStreamer-1.0.v3

Almost :)
>+		if (!gst_buffer_map (buffer, &extractor->info, GST_MAP_READ)) {
>+			gst_sample_unref (sample);
>+			return FALSE;
>+		}
>+
>+		extractor->media_art_buffer = extractor->info.data;
>+		extractor->media_art_buffer_size = extractor->info.size;
> 		extractor->media_art_buffer_mime = gst_structure_get_name (caps_struct);

I think here we should save the buffer (or sample containing the buffer) in the extractor as well (don't need a ref if we hold on to the tag list, but can take a ref for clarity).

>+	buffer = gst_buffer_new ();
>+	gst_buffer_unmap (buffer, &extractor->info);
>+	gst_buffer_unref (buffer);

This isn't how it's supposed to be used, you need to unmap the buffer you did the map() with before.
Comment 8 Javier Jardón (IRC: jjardon) 2012-09-17 03:54:47 UTC
Created attachment 224471 [details] [review]
Port-discoverer-backend-to-GStreamer-1.0.v4

Another try;) I store the GstSample as you commented
Comment 9 Matthias Clasen 2012-09-17 21:45:59 UTC
Anybody up for reviewing this patch ? tpm ?
Comment 10 Martyn Russell 2012-09-18 07:25:18 UTC
I will happily review it if there is no more to add from Tim-Philipp Müller?
Comment 11 Tim-Philipp Müller 2012-09-18 15:19:15 UTC
Comment on attachment 224471 [details] [review]
Port-discoverer-backend-to-GStreamer-1.0.v4


>+	GstMapInfo      info;

Is this variable still used now that we have extractor->info ?


>@@ -2024,7 +2033,9 @@ tracker_extract_gstreamer (const gchar          *uri,
> 
> 	g_free (extractor->media_art_artist);
> 	g_free (extractor->media_art_title);
>-	/* Embedded media art buffer is owned and freed by the GstTagList */
>+	buffer = gst_sample_get_buffer (extractor->sample);
>+	gst_buffer_unmap (buffer, &extractor->info);
>+	gst_sample_unref (extractor->sample);
> 
> 	gst_tag_list_free (extractor->tagcache);

I wonder if this needs to be something like:

  if (extractor->sample != NULL) {
    .. unmap ..
    gst_sample_unref (extractor->sample);
  }

so we don't unref a NULL sample if there wasn't any cover art?


Ultimately, the proof is in the pudding I guess. Has it been tested with files with and without embedded cover art?
Comment 12 Javier Jardón (IRC: jjardon) 2012-09-18 16:58:44 UTC
Created attachment 224647 [details] [review]
Port-discoverer-backend-to-GStreamer-1.0.v5

Updated patch. Unfortunatelly Im abroad and I do not have any file to test sorry
Comment 13 Matthias Clasen 2012-09-21 22:17:25 UTC
Dropping off 3.6 blocker list; if we get the patch landed, thats great, if not, lets get it in for 3.6.1
Comment 14 Matthias Clasen 2012-10-17 00:39:24 UTC
Any update on testing this patch ?
Comment 15 Martyn Russell 2012-10-17 11:30:24 UTC
Comment on attachment 224647 [details] [review]
Port-discoverer-backend-to-GStreamer-1.0.v5

Hi Javier,

>@@ -2024,7 +2033,11 @@ tracker_extract_gstreamer (const gchar          *uri,
> 
> 	g_free (extractor->media_art_artist);
> 	g_free (extractor->media_art_title);
>-	/* Embedded media art buffer is owned and freed by the GstTagList */
>+	if (extractor->sample) {
>+		buffer = gst_sample_get_buffer (extractor->sample);
>+		gst_buffer_unmap (buffer, &extractor->info);
>+		gst_sample_unref (extractor->sample);
>+	}
> 
> 	gst_tag_list_free (extractor->tagcache);
> 
>-- 
>1.7.11.4
>

So my only concern about this patch is why we're using the extractor struct to keep the sample and info information?

AFAICS, they are both used ONLY in one function, the get_embedded_media_art() function. Can't this be declared locally in the scope of that function?

Other than that, please go ahead and commit Javier. Thank you all.
Comment 16 Tim-Philipp Müller 2012-10-17 11:41:26 UTC
The reason for that is:

+				extractor->media_art_buffer = extractor->info.data;
+				extractor->media_art_buffer_size = extractor->info.size;

This points to data into the coverart image buffer (sample). This data is only valid as long as that buffer is mapped, technically. So we keep the buffer mapped, but that means we need to unmap it again later, for which we need to keep the sample+info structs around as well.
Comment 17 Martyn Russell 2012-10-24 15:23:27 UTC
(In reply to comment #16)
> The reason for that is:
> 
> +                extractor->media_art_buffer = extractor->info.data;
> +                extractor->media_art_buffer_size = extractor->info.size;
> 
> This points to data into the coverart image buffer (sample). This data is only
> valid as long as that buffer is mapped, technically. So we keep the buffer
> mapped, but that means we need to unmap it again later, for which we need to
> keep the sample+info structs around as well.

Right you are. Perhaps it's time for new glasses here :)

Pushed.