GNOME Bugzilla – Bug 680424
Port to gstreamer 1.0
Last modified: 2012-10-24 15:23:56 UTC
See https://live.gnome.org/GnomeGoals/PortToGstreamer1
Created attachment 222075 [details] [review] Port-discoverer-backend-to-GStreamer-1.0 This patch only ports the dicoverer backend (the one by default)
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 >
Created attachment 222180 [details] [review] Port-discoverer-backend-to-GStreamer-1.0.v2 Updated patch with your comments
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.
Javier: Do you plan to update your patch based on comment 4?
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 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.
Created attachment 224471 [details] [review] Port-discoverer-backend-to-GStreamer-1.0.v4 Another try;) I store the GstSample as you commented
Anybody up for reviewing this patch ? tpm ?
I will happily review it if there is no more to add from Tim-Philipp Müller?
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?
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
Dropping off 3.6 blocker list; if we get the patch landed, thats great, if not, lets get it in for 3.6.1
Any update on testing this patch ?
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.
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.
(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.