GNOME Bugzilla – Bug 720218
Proposed changes to the librygel-server MediaObject class hierarchy
Last modified: 2015-02-13 12:45:09 UTC
Created attachment 263948 [details] [review] Proposed changes to the MediaObject class hierarchy Attached is a patch that refactors the MediaObject class hierarchy to (1) isolate assumptions about file-based/user-supplied content to a new MediaFileItem class, (2) incorporate a MediaResource class to represent a MediaObject's constituent resources. This change (and related changes which I'll post in separate bugs) are significant. So a document is attached that attempts to explain the changes we're proposing for librygel-server in more detail. Comments and feedback are welcome.
Created attachment 263949 [details] Description/background document for the proposed librygel-server changes
General question: Can we reuse the class diagram in the document for our current documentaion?
Sure. Or I can forward/attach the OpenOffice Draw file. That'll probably be better for import/tweaking...
This is still quite big to wrap one's head around it, at a first glance, the MediaFileitem change seems reasonable.
Yeah - no doubt it's a jumbo-sized change. Believe it or not, I did resist changing some things. e.g. I really wanted to replace the MediaObject.uris array with a uri. But it would have only caused more changes. Let me know if there's something else I can do to help break it up or if there's a more interactive forum we could use. BTW, there are a few (much smaller) follow-on changes to deal with live (and indeterminate size) content. I'll try to provide some description along with the changes.
MediaObject.uris is indeed something that needs adressing, but agreed, that would complicate this patch even more. I'll try to get at least some more in-depth feedback by the end of the week, but unfortunately my time is somewhat limited currently and FOSDEM wasn't as productive as I hoped (haha. As if that ever worked out, but one can try)
(In reply to comment #0) > Created an attachment (id=263948) [details] [review] > Proposed changes to the MediaObject class hierarchy > > Attached is a patch that refactors the MediaObject class hierarchy to (1) > isolate assumptions about file-based/user-supplied content to a new > MediaFileItem class, (2) incorporate a MediaResource class to represent a > MediaObject's constituent resources. If you are talking about 2 changes in the commit log, it indicates that you are putting at least 2 seperate patches into one. Which would also explain why you had to use such a vague shortlog/summary. All this makes reviewing a bit difficult, as you can see from Jens's comment. > This change (and related changes which I'll post in separate bugs) are > significant. So a document is attached that attempts to explain the changes > we're proposing for librygel-server in more detail. Unless document contains pictures, would be rather better to use bugzilla itself for explanation. It would avoid people having to click many things to just to get to justification. Also the more info you can provide in the commit logs themselves, the better but from what I can tell, you are already doing that very nicely. Talking of commit log, please keep in mind that its going to be read by most people *after* your patch is merged and at that time its no longer a proposal so I'd avoid using that word as its clear that any unmerged patches are almost always proposals and you can use the same space for describing the change itself. Yeah, I'm a perfectionist and therefore a big nitpicker. :) > Comments and feedback are welcome. Good, hopefully also negative ones like this one. :)
Re: commit log/patches Yeah, there are a lot more than 2 patches here. I'll try to come up with something that better qualifies as a commit log message if/when the time comes (e.g. the squash commit log message from the branch merge). Re: document There are a couple diagrams in the document. But I'm happy to try putting the information into the bugzilla here and/or convert it to HTML if you think it's practical/useful. Re: feedback Yes - definitely all feedback is welcome (and I'm not even sure your's qualifies as "negative"… ;^J). I consider myself a nitpicker too (and think there are many that will vouch for that). These submissions are either a nitpicker's dream or nightmare, depending on how you look at it. (or maybe a cure?) Above all else, I want to make these changes consistent with your groundwork and maintainable. So I expect to "turn the crank" at least a couple of times before it's 100%.
Review of attachment 263948 [details] [review]: I agree with Zeeshan, splitting this into "Introduce MediaFileItem" and "Add MediaResource" class should make since easier; the first patch would be rather simple apart from moving a lot of code into the new class, which leaves less clutter in the "more complicated" second patch. That said, do I see it correctly that this patch removes the transcoding capabilities without adding a replacement? Isn't that also unrelated from moving to MediaResource? ::: src/librygel-server/rygel-media-resource.vala @@ +132,3 @@ + } + + public void apply_didl_lite (DIDLLiteResource didl_resource) { Isn't that more of a "_from_didl_lite_resource" constructor? @@ +158,3 @@ + } + + public DIDLLiteResource serialize (DIDLLiteResource didl_resource) { Just for clarification: This returns the DIDLLiteResrource for easy function call chaining? @@ +285,3 @@ + + public string to_string () { + var strbuf = new StringBuilder (); nitpick: Could start with new StringBuilder(name);
things, not "since". also, review contains some random comments.
Review of attachment 263948 [details] [review]: ::: src/librygel-server/rygel-media-resource.vala @@ +158,3 @@ + } + + public DIDLLiteResource serialize (DIDLLiteResource didl_resource) { Yeah - MediaResource allows resources to be enumerated/communicated outside the context of serializing a upnp item. But when the time comes to insert the resource into a serialization, this is the function that does the job. (not sure I'm answering your question - let me know if not...) @@ +285,3 @@ + + public string to_string () { + var strbuf = new StringBuilder (); Agreed. I'd make the same comment. I'll correct.
Review of attachment 263948 [details] [review]: (sorry for the 2 posts - still getting to know this review system) Re: Splitting the patch Let me investigate a bit if there's a relatively simple way to split this up even more. Re: transcoding functionality Transcoding is still fully-supported with these changes. The engine just expresses additional MediaResource objects for the transcode profiles it supports. The adapted version of the GstMediaEngine on bug 720220 (which retains all the transcode functionality of the current GstMediaEngine) illustrates how transcodes can be represented & streamed by a ME.
Created attachment 271055 [details] [review] Refactoring of the MediaObject class hierarchy This patch refactors the MediaObject class hierarchy to isolate assumptions about file-based/user-supplied content to a new MediaFileItem class.
Created attachment 271056 [details] [review] Introduction of a MediaResource class This patch introduces a MediaResource class representing a MediaObject's constituent resources and migrates existing resource-related logic to utilize the class for primary and secondary resource representations.
OK - these two new attachments effectively split up what was in the single patch before. Note that the patches can't be applied independently (without engineering). The MediaResource patch depends upon the MediaObject class hierarchy changes. And The dependent/blocked bugs depend on both sets of changes. Anyway, hope this is more "digestible"...
Back from Spring Break here. Any takers for reviewing?
on it, but it's slow progress due to time constraints
Review of attachment 271056 [details] [review]: Re transcoding: Right; though that means we really need to get everything in together which I rather liked to avoid since it's large. ::: src/librygel-server/rygel-media-resource.vala @@ +14,3 @@ + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + * Please use the same copyright header as the rest of Rygel. @@ +65,3 @@ + // But both proved to be problematic in their current form. This class can be + // refactored if/when these classes are made more more flexible. For now, this class + // needs to serve the needs of Rygel first and foremost... Please explain "problematic". We do have the full stack under our control and can fix it instead of duplicating things. Right now the only nice reason I could think of is not using string[] for PlaySpeeds, which you don't. @@ +101,3 @@ + } + + public static string []? copy_speeds (string? [] src) { Ugly but probably unavoidable @@ +114,3 @@ + } + + public string get_name () { Why don't you make this a construct-write-read-only property instead? public string name { construct; get; } @@ +120,3 @@ + private HashMap<string,string> property_table = new HashMap<string,string> (); + + public void set_custom_property (string ? name, string ? value) { Are you allowing null key and value for a reason here? @@ +250,3 @@ + } + + switch (transfer_mode) { Please add constants for those strings. @@ +286,3 @@ + public string to_string () { + var strbuf = new StringBuilder (); + strbuf.append (name).append_unichar ('('); You can combine these two ::: src/librygel-server/rygel-playlist-item.vala @@ +51,3 @@ + internal override void add_resources (HTTPServer server) { + // PlaylistItem doesn't add secondary resources + } Why doesn't the base class have an empty virtual function? ::: src/librygel-server/rygel-source-connection-manager.vala @@ -46,3 @@ - this.source_protocol_info += protocol_info.to_string (); - } - this.source_protocol_info += ","; Doesn't this remove the protocol info handling (maybe this is already superseded by some other patch)
Review of attachment 271055 [details] [review]: While we're at it, let's get rid of the uris property in MediaObject alltogether. It has really bad semantics. ::: src/librygel-server/rygel-media-file-item.vala @@ +250,3 @@ + * + * Note: Implementations should add both internal/file-based resources and HTTP-accessible + * resources to the MediaResource list. Missing function parameter description ::: src/librygel-server/rygel-media-item.vala @@ -42,3 @@ - // Resource info - public string mime_type { get; set; } - public string dlna_profile { get; set; } I suppose this is moved here already due to the MediaResource change? @@ -114,3 @@ - // Live media items need to provide a nice working implementation of this - // method if they can/do not provide a valid URI - public virtual DataSource? create_stream_source (string? host_ip = null) { Why is this removed and not made abstract? @@ -136,3 @@ - } - - public abstract bool streamable (); Same for streamable; why is this removed? @@ -143,3 @@ - - internal int compare_transcoders (Transcoder transcoder1, - public abstract bool streamable (); Would it make sense to have the new-gen MediaItems transcodable as well? ::: src/librygel-server/rygel-media-object.vala @@ +44,3 @@ public string upnp_class { get; construct set; } + public string date { get; set; } + public string creator { get; set; } This change seems a bit unrelated to the rest? Fall-out from the refactoring? (I agree it makes sense, though)
Review of attachment 271056 [details] [review]: ::: src/librygel-server/rygel-media-resource.vala @@ +65,3 @@ + // But both proved to be problematic in their current form. This class can be + // refactored if/when these classes are made more more flexible. For now, this class + // needs to serve the needs of Rygel first and foremost... Yeah - I would hope so. I just didn't have time/context to take the issues on myself. I'll see how many of the details I can recall. But I know that any case where I instantiated ProtocolInfo and put it into a container (GLib or Gee) and tried to pass the container out of a function, elements were corrupted and/or segfaults occurred. I was never able to track down the root cause. But the backtraces seemed to indicate that glib was attempting to deep-copy the ProtocolInfo objects and failing. And instantiating DIDLLiteResource (and related classes) outside of the context of parsing a didl-lite document doesn't seem possible. @@ +114,3 @@ + } + + public string get_name () { I thought I had it that way. Trying to recall if I was having property issues. I'll change it (assuming it's not being reassigned somewhere...) and look for other opportunities to codify read-onlyness... @@ +120,3 @@ + private HashMap<string,string> property_table = new HashMap<string,string> (); + + public void set_custom_property (string ? name, string ? value) { That method is gone in the latest version (along with get_custom_property() and get_custom_property_names()). Those were an early but were unused/unjustified. @@ +250,3 @@ + } + + switch (transfer_mode) { ok - will do. @@ +286,3 @@ + public string to_string () { + var strbuf = new StringBuilder (); + strbuf.append (name).append_unichar ('('); Yeah - I'll do a complete audit of blank StringBuilder constructors and see how many of
Review of attachment 271055 [details] [review]: ::: src/librygel-server/rygel-media-file-item.vala @@ +250,3 @@ + * + * Note: Implementations should add both internal/file-based resources and HTTP-accessible + * resources to the MediaResource list. OK - I'll improve the description and add the parameter. ::: src/librygel-server/rygel-media-item.vala @@ -42,3 @@ - // Resource info - public string mime_type { get; set; } - public string dlna_profile { get; set; } Yeah - as an Item can now have multiple MediaResources - each with a distinct mime_type, dlna_profile, etc. @@ -114,3 @@ - // Live media items need to provide a nice working implementation of this - // method if they can/do not provide a valid URI - public virtual DataSource? create_stream_source (string? host_ip = null) { create_stream_source is pushed up to MediaObject - which allows it to be used in MediaContainer - which uses create_stream_source() to generate DataSources for container playlists. This was useful as it eliminated HTTPPlaylistHandler - reducing/simplifying some of the HTTP code changes. @@ -136,3 @@ - } - - public abstract bool streamable (); The reason is different in this case. streamability is resource-specific - so this is now MediaResource.is_streamable(). @@ -143,3 @@ - - internal int compare_transcoders (Transcoder transcoder1, - Transcoder transcoder2) { This is already supported and retrofitted. We wanted to ensure all existing functionality was retained. Have a look at GstMediaEngine.get_resources_for_item (MediaObject object). The existing GST transcoders should be working as before (functionally). They are just represented by MediaResources instead of Transcoders. ::: src/librygel-server/rygel-media-object.vala @@ +44,3 @@ public string upnp_class { get; construct set; } + public string date { get; set; } + public string creator { get; set; } Doug added date and creator to the ODID MediaServer's SQL database so they are searchable. I believe this was done to pass some DLNA certification tests. See ODID.MediaCache.create_normal_object and update_guarded_object(). These are modelled after the MediaExport versions, which take MediaObjects. But it looks like more date/creator-related logic in MediaItem should be moved into MediaObject to finish the job. Perhaps this can be taken into its own patch...
(In reply to comment #20) > Review of attachment 271056 [details] [review]: > > ::: src/librygel-server/rygel-media-resource.vala > @@ +65,3 @@ > + // But both proved to be problematic in their current form. This class > can be > + // refactored if/when these classes are made more more flexible. For now, > this class > + // needs to serve the needs of Rygel first and foremost... > > Yeah - I would hope so. I just didn't have time/context to take the issues on > myself. > > I'll see how many of the details I can recall. But I know that any case where I > instantiated ProtocolInfo and put it into a container (GLib or Gee) and tried > to pass the container out of a function, elements were corrupted and/or > segfaults occurred. I was never able to track down the root cause. But the > backtraces seemed to indicate that glib was attempting to deep-copy the > ProtocolInfo objects and failing. And instantiating DIDLLiteResource (and > related classes) outside of the context of parsing a didl-lite document doesn't > seem possible. Argh, you're right of course. Hrm...
(In reply to comment #22) > (In reply to comment #20) > > Review of attachment 271056 [details] [review] [details]: > > > > ::: src/librygel-server/rygel-media-resource.vala > > @@ +65,3 @@ > > + // But both proved to be problematic in their current form. This class > > can be > > + // refactored if/when these classes are made more more flexible. For now, > > this class > > + // needs to serve the needs of Rygel first and foremost... > > > > Yeah - I would hope so. I just didn't have time/context to take the issues on > > myself. > > > > I'll see how many of the details I can recall. But I know that any case where I > > instantiated ProtocolInfo and put it into a container (GLib or Gee) and tried > > to pass the container out of a function, elements were corrupted and/or > > segfaults occurred. I was never able to track down the root cause. But the > > backtraces seemed to indicate that glib was attempting to deep-copy the > > ProtocolInfo objects and failing. And instantiating DIDLLiteResource (and > > related classes) outside of the context of parsing a didl-lite document doesn't > > seem possible. > > Argh, you're right of course. Hrm... It would certainly be nice to make the gupnp-av classes more usable outside of the didl-lite (I can try and help with that) as I don't love having to replicate the structures. But I'm worried that making this gupnp-av support a pre-condition to integrating this patch as it will further complicate integration/testing. Could we (as I think I mentioned in the comments) retrofit this logic if/when gupnp-av supports this usage model? I think the semantics should line up well enough that it won't be a major issue.
Yes, we can do that later. I started splitting this patch so that it doesn't break current code.
Created attachment 279479 [details] [review] Refactoring of the MediaObject class hierarchy
Created attachment 279480 [details] [review] Introduction of a MediaResource class
Created attachment 279629 [details] [review] Refactoring of the MediaObject class hierarchy FIXME: need commit message. (Please also double check the author and subject.)
Created attachment 279631 [details] [review] Introduction of a MediaResource class FIXME: need commit message. (Please also double check the author and subject.)
Created attachment 279632 [details] [review] all: Refactoring of the MediaObject class hierarchy Introduce a MediaFileItem class that represents a single file Signed-off-by: Jens Georg <mail@jensge.org>
So, I've done a bit of rebasing and squashing. Attachment 279632 [details] is a combination of the MediaItem -> MediaFileItem renaming parts of this bug and bug 720224. Bug 720224's patch was updated on top of that and now only contains the resource-related parts. Likewise, the two original patches were rebased on top of that patch.
Review of attachment 279631 [details] [review]: ::: src/librygel-server/rygel-audio-item.vala @@ -110,2 +110,1 @@ return didl_item; - } This looks broken, like a merge that went wrong (I checked, it's in the original patch as well) @@ +120,3 @@ } + + internal override void add_resources (HTTPServer server) { I'm kind-of missing the original definition of this method. Where should it go, MediaItem/MediaObject/MediaFileItem? ::: src/librygel-server/rygel-media-object.vala @@ +55,2 @@ // and make the uri property single-value. public Gee.ArrayList<string> uris; This can go if we add resources, I think. MediaFileItem would have to "fake" it working on the resources, but I can do that after we merge this. @@ +260,3 @@ + public void serialize_resource_list (DIDLLiteObject didl_object, + HTTPServer http_server) + throws Error { This looks roughly similar to the get_resource_list_for_server method; I'm wondering whether one might be able to use the other @@ +264,3 @@ + foreach (var res in get_resource_list ()) { + if (res.uri == null || res.uri == "") { + // Any resource without a URI will get a HTTP resource-based URI Why is this part not a method on the HTTPServer? @@ +279,3 @@ + continue; + } + if (protocol != "internal" || http_server.is_local ()) { While that boolean expression does what the comment claims, it's not straigt forward to understand. @@ +291,3 @@ + var scheme = Uri.parse_scheme (uri); + if (scheme == null) { + throw new MediaFileItemError.BAD_URI (_("Bad URI: %s"), uri); As this is thrown here, it should be a MediaObjectError now. ::: src/librygel-server/rygel-media-resource.vala @@ +1,1 @@ +/* This file has mainly style issues with missing "this." prefix. @@ +25,3 @@ + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. Remove BSD clause, please. ::: src/librygel-server/rygel-source-connection-manager.vala @@ -37,3 +39,2 @@ this.av_transport_id = -1; this.direction = "Output"; - Is this replaced in a later patch?
Review of attachment 279631 [details] [review]: ::: src/librygel-server/rygel-audio-item.vala @@ +120,3 @@ } + + internal override void add_resources (HTTPServer server) { Ah sorry, it's in the other patch, nevermind.
Review of attachment 279631 [details] [review]: ::: src/librygel-server/rygel-media-object.vala @@ +262,3 @@ + throws Error { + // Note: Intentionally not using get_resource_list_for_server() to avoid a copy + foreach (var res in get_resource_list ()) { Also I wonder why I missed that comment ;) @@ +316,3 @@ + public abstract DataSource? create_stream_source_for_resource + (HTTPRequest request, MediaResource resource) throws Error; + Why does this live in MediaObject rather than in MediaItem?
Review of attachment 279631 [details] [review]: ::: src/librygel-server/rygel-media-resource.vala @@ +67,3 @@ + // needs to serve the needs of Rygel first and foremost... + + public MediaResource (string name) { Does "name" have any meaning? I see the use of "primary" which wraps the old uris[0] and the two names for the playlist resources on containers, but can't really see a scheme for it
Review of attachment 279631 [details] [review]: sorry - missed the "name" question. ::: src/librygel-server/rygel-media-resource.vala @@ +67,3 @@ + // needs to serve the needs of Rygel first and foremost... + + public MediaResource (string name) { Sorry - missed this comment. The name field is required to be unique when multiple resources are returned for an item from MediaEngine.get_resources_for_item(). * The order of MediaResources in the returned List should be from most-preferred to * least-preferred and each must have a unique alphanumeric "name" field. And since the resource passed to ME.create_data_source_for_resource() must come from get_resources_for_item(), the MediaEngine can use the name field to recognize the resource (since its the one that sets the name field). (we should probably document the fact that object identity cannot be used - so that MediaServer-cached/reconstituted MediaResources can be passed to create_data_source_for_resource().
Review of attachment 279631 [details] [review]: Responding to a couple question I missed. ::: src/librygel-server/rygel-media-object.vala @@ +55,2 @@ // and make the uri property single-value. public Gee.ArrayList<string> uris; sounds good. @@ +262,3 @@ + throws Error { + // Note: Intentionally not using get_resource_list_for_server() to avoid a copy + foreach (var res in get_resource_list ()) { Glad I put that comment there or I would have forgotten why as well... @@ +264,3 @@ + foreach (var res in get_resource_list ()) { + if (res.uri == null || res.uri == "") { + // Any resource without a URI will get a HTTP resource-based URI Not sure what you're referring to - the "if" or the if block... @@ +279,3 @@ + continue; + } + if (protocol != "internal" || http_server.is_local ()) { How about: if ((protocol == "internal") && !http_server.is_local ()) { // Exclude internal resources when request is non-local } else { // Include URIs accessible via the HTTP server DIDLLiteResource didl_resource = didl_object.add_resource (); res.serialize (didl_resource); } @@ +291,3 @@ + var scheme = Uri.parse_scheme (uri); + if (scheme == null) { + throw new MediaFileItemError.BAD_URI (_("Bad URI: %s"), uri); Agreed. @@ +316,3 @@ + public abstract DataSource? create_stream_source_for_resource + (HTTPRequest request, MediaResource resource) throws Error; + As I think you guessed, this is to allow MediaContainers to generate DataSources. Specifically, MCs can create PlaylistDataSources in both DIDL_S and M3U flavors.
Code imported from github into master