[gstreamer-bugs] [Bug 625944] New GstDiscoverer helper library

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=625944
  GStreamer | gst-plugins-base | git

--- Comment #35 from Edward Hervey <bilboed at gmail.com> 2010-09-18 10:36:29 UTC ---
(In reply to comment #33)
> - use _discoverer_uri() and not _discoverer_uri_sync(), that follows glib/gio
> style
> 
  Makes sense. Maybe discoverer_uri_async() then for the alternative ?

> - really put it in pbutils, especially now that the video dependency is gone.
> all these micro libs are not going to reduce the memory footprint and
> performance of gstreamer (but the same goes for all the micro plugins which
> should better be grouped...)

  It will still need it for compilation though, the only thing removed is
exporting libgstvideo API in the headers. Will move it in next commits.

> 
> - IMHO GstDiscovererResult should be a simple enum and not flags. error is an
> unspecified error and the 4 other failures are specified errors. they feel
> disjoint

  Already changed that locally, will come in next push.

> 
> - What about the compatiblity with the GstToc stuff? Edward, did you look at
> that patch and think about the relationship? :) It would be bad if discoverer
> caused problems for the toc stuff or the other way around and we have one of
> the APIs stable already.

  We can add a _discoverer_uri_chapter{_async}(uri, chapter) later on. It would
need to take subtlely different code paths since it would require prerolling
and then going to the required chapter.

> 
> 
> Other than that I think it's all ok and can be merged.

(In reply to comment #34)
> Oh, and:
> 
> - Subtitle support? :) (can be added later but would still be nice)

  Argh, right. The advantage with having everything sealed in the new API is
that we can trivially add it. I don't see any issues with that (it's just a
different media type, but has behaviour similar to audio/video/container
streams).
  I'm not that aware of the various properties of a subtitle stream, so would
need some input about what would be needed to export in a
GstDiscovererSubtitleStreamInfo.

> 
> - ownership of the return values for accessors is not obvious, at least for
> gst_discoverer_container_info_get_streams, gst_discoverer_stream_info_get_misc,
> gst_discoverer_stream_info_get_tags, gst_discoverer_stream_info_get_caps,
> gst_discoverer_stream_info_get_next, gst_discoverer_stream_info_get_previous.
> Does the caller get a copy/ref? The code says "no" but the docs doesn't mention
> this and the return types are not marked as const either. It makes sense to
> have them still owned by the info object but it should be documented at least.

  I've been doing fixups for those in my local branch (basically const-ing when
it would require a copy, or adding a ref when possible), will comme in my next
push.

  Thanks for the reviews

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.