GNOME Bugzilla – Bug 710628
Prepare Rygel core for media-engines handling preprocessed resources
Last modified: 2014-06-14 16:07:32 UTC
Created attachment 257836 [details] [review] Suggested changes Patch to modify Rygel to support preprocessed resources (DTCP etc)
Review of attachment 257836 [details] [review]: General issues: - I'm kind of confused. There are now several ways to append resources to objects, right? The old one and the new one using MediaResource, right? Wouldn't it be more consistent to just provide one way to do this? - I need some explanation on the HTTP seeking changes, the necessities for this aren't really obvious to me. - I have the feeling this patch combines several fixes into one, could this be split somehow? I'm kind-of lost. ::: data/rygel.conf @@ +50,3 @@ +# Possible values - image-upload;av-upload;audio-upload +# This will take effect only if allow-upload is set to true +upload-option=image-upload;av-upload;audio-upload Why did you move this into the general config, shouldn't this more be related to the actual server plug-in? @@ +114,2 @@ [Tracker] +enabled=false why? @@ +131,3 @@ # +# Since we are disabling non av-upload disabling music and audio folders from harvesting. +uris=@VIDEOS@ Can't do this for the default config, that will confuse our "hobbyist" users. ::: src/librygel-core/rygel-description-file.vala @@ +212,2 @@ if (allow_upload) { + foreach (var option in upload_options) { Did you intentionally invalidate the plugin's upload capabilities? ::: src/librygel-server/rygel-connection-manager-protocolinfo.vala @@ +41,3 @@ + private static ConnectionManagerProtocolInfo cm_protocol_info = null; + + internal class CMSProtocolInfo { Why can't you re-use the stuff from GUPnP-AV here? Couldn't we add this to GUPnP-AV instead? Feels like doubled effort. ::: src/librygel-server/rygel-content-directory.vala @@ +471,3 @@ } + private async void get_all_items () { Why is this called get_all_items() when it updates the protocol_info? ::: src/librygel-server/rygel-dlna-playspeed.vala @@ +208,3 @@ + } + + private void parse(string speed) throws DLNAPlaySpeedError { I think we have some code for this that just needs moving ::: src/librygel-server/rygel-dtcp-cleartext-byte-seek.vala @@ +155,3 @@ + +public static const string DTCP_CLEARTEXT_RANGE_RESPONSE_HEADER = "Content-Range.dtcp.com"; + One file per class, please ::: src/librygel-server/rygel-http-get.vala @@ +41,2 @@ private const string TRANSFER_MODE_HEADER = "transferMode.dlna.org"; + private const string SERVER_NAME = "CVP2-RI-DMS"; Why is that necessary? ::: src/librygel-server/rygel-http-seek.vala @@ -23,1 +23,1 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. Isn't that class completely useless now? I still can't understand why you had to move the members to the deriving class and had to rename all of them ::: src/librygel-server/rygel-item-updater.vala @@ +110,3 @@ + // Remove any leading or trailing spaces for the corresponding text node. + private static string formatTagValues (string tag_values){ Isn't that a completely unrelated fix that could go in separately? Also, what's the reason for this? ::: src/librygel-server/rygel-media-item.vala @@ +73,3 @@ + // Note: MediaResources are ordered from most- to least-preferred + public Gee.List<MediaResource> media_resources { get; set; default = null; } + Don't add this as a property, this is already very annoying with uris, add some generic add_resource, remove_resource etc. helper functions. Otherwise it gets quite unusable from GObject-introspection bindings. Also, why is this on Item and not Object? @@ +336,1 @@ protocol_info.dlna_flags = DLNAFlags.DLNA_V15 | Well yes. Until lately we knew that exactly. ::: src/librygel-server/rygel-relational-expression.vala @@ +40,3 @@ internal const string CAPS = "@id,@parentID,@refID,upnp:class," + "dc:title,upnp:artist,upnp:album," + + "dc:creator,@childCount"; Is there any reason to remove upnp:createClass from the search caps?
For anyone not aware of the source of this patch (namely, doug, parthi, and myself at CableLabs), this patch represents a work-in-progress for changes to Rygel core to support "device-sourced" content and improved DLNA features/compliance. Thanks Jens for getting this rolling. I'll see if I can respond inside the review system (apologies if I mess up)...
Review of attachment 257836 [details] [review]: [OK - my first attempt to add review comments] Re: Appending resources to objects Yeah, there are 2 ways to add resources in this WIP. But there should just be just one when we're done cleaning things up. I'm advocating MediaResource for this since they can be created/managed/munged to add methods and such (similar to MediaItems, MediaObjects, etc), have this important "name" element for identification, and can be created independent of a gupnp Serializer. But I think the burden is on us to prove/demonstrate that. Re: HTTP seeking changes Are you referring to the time-seek, byte-seek, or dtcp cleartext byte-seek changes? Let me see if I can at least explain the separation of HTTPSeek into HTTPSeekRequest and HTTPResponseElement: To properly support the handling of content that can be accessed in the time domain (TimeSeekRange.dtcp.com) or DTCP cleartext domain (Range.dtcp.com), a DLNA server is required provide feedback in the request response that reflects the actual content returned - esp for content that supports multiple domains. e.g. If a DMS supports both TimeSeekRange and Range requests, then the TimeSeekRange response needs to indicate both the time and data range corresponding to the returned data. And note that the *requested* time range may not match the *responded* time range (since a DMS is required to return data starting at a "decoder-friendly point"). Note that Rygel with the GstMediaServer doesn't had to worry about multiple domains since the primary (non-transcoded) is advertised to support only byte-based seek (ORG_OP's a-val of 0, b-val 1). And transcoded content is advertised to support only time-based seek (a-val 1, b-val 0). It's only when you (a) support both byte-based and time-based seek on the same content binary (a/b-val both 1), (b) you throw something in like DTCP, where things get "interesting". (Note, though, that GstMediaEngine should be returning adjusted time vals in the TimeSeekRange response regardless) Anyway, since knowledge of the raw content data is (presumably) the exclusive domain of the DataSource/MediaEngine, the request needs to be passed there (as HTTPSeek always had). But the DataSource needs to have the ability to provide feedback which is incorporated into the response headers. So I introduced the HTTPResponseElement concept and DataSource.preroll() for this. Note that initially, I thought a single object containing the request/response would work for both seek and speed requests (and even coded it that way). But DTCP support became problematic since the requirements for Content-Range.dtcp.com inclusion meant that it needed to be included for byte-based and time-based requests - and speed requests. So all these classes would suddenly need to know about Content-Range.dtcp.com, which was lame (we tried it). It just made more sense to allow seek/speed requests to return an arbitrary set of responses. And negative PlaySpeed throws another whole wrinkle into things that I'll resist going into here (since this is already too long). Anyway, have a look at the ODIDMediaEngine.preroll() for an example (in issue 710704). Re: Multiple fixes, being lost, etc Yeah, I wish this change wasn't so large. It's a bit challenging to split up these changes due to the interdependencies. One option might be to separate the changes between the upnp action support/item metadata and the request processing side (libsoup/HTTPServer). And think we owe anyone reviewing some guidance on what we're trying to accomplish. I'll definitely work on that prior to submitting a revised patch. ::: data/rygel.conf @@ +114,2 @@ [Tracker] +enabled=false This file represents the config we've been testing (and MediaExport is hotwired a bit in this rev - since we were using our own MediaServer) We basically had disabled everything we didn't need for our testing. @@ +131,3 @@ # +# Since we are disabling non av-upload disabling music and audio folders from harvesting. +uris=@VIDEOS@ Yeah, part of the hot-wiring we've done to MediaExport means that only Videos are working in this rev. We're working to remedy this now. ::: src/librygel-server/rygel-dtcp-cleartext-byte-seek.vala @@ +155,3 @@ + +public static const string DTCP_CLEARTEXT_RANGE_RESPONSE_HEADER = "Content-Range.dtcp.com"; + OK, will do. Was just enjoying the additional latitude Vala allows. Do you guys have coding conventions/rules posted, btw? ::: src/librygel-server/rygel-media-item.vala @@ +73,3 @@ + // Note: MediaResources are ordered from most- to least-preferred + public Gee.List<MediaResource> media_resources { get; set; default = null; } + Yeah, struggled with the property/member option here. We'll move it to MediaObject as a regular memvar with add/remove semantics. @@ +336,1 @@ protocol_info.dlna_flags = DLNAFlags.DLNA_V15 | Should these flags maybe be retrieved from the MediaServer, similar to how HTTPServer.get_protocol() is used?
Review of attachment 257836 [details] [review]: ::: src/librygel-server/rygel-http-seek.vala @@ -23,1 +23,1 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. Ah - realized I may not have answered this. A couple reasons why I took the range values out of the base class. (a) TimeSeekRange and Range.dtcp.com responses must have both a byte and/or time range and/or cleartext range in some cases (as I described above). I did attempt to maintain the base class to contain the byte range. But this became problematic as responses only contain byte ranges in some cases. (both for Range.dtcp.com and TimeSeekRange) (I did end up realizing that having the same class encapsulate request and response was problematic after this though) (b) the base class tried to do range validation without consideration of the range unit. Unfortunately, the rules for the TimeSeekRange depend on the direction of play *and* on the speed. e.g. for negative rates, range "x-y" must have time x > time y. And at any non-1.0 rate, x or y is allowed to be larger than the content duration. So at that point, having range values in the base class with both interpretation of range units and validation in the subclasses doesn't seem to provide any value that I could see. So history aside, (b) is the primary reason. And open ranges (ranges specified as "x-") will become more important with non-fixed resources as their interpretation also varies according to the request type and direction. Re: The existence of HTTPSeekRequest The type still provides value in the sense that a DataSource.preroll() can be declared to take an "HTTPSeekRequest". The DataSource just has to act on the seek in a type-specific way (as it always had to anyway). The DataSource just uses the "object is type" test instead of HTTPSeekType. (let me know if there are caveats with that, btw...)
::: src/librygel-core/rygel-description-file.vala @@ +212,2 @@ if (allow_upload) { + foreach (var option in upload_options) { Did you intentionally invalidate the plugin's upload capabilities? Response: Yes, This change allows granular upload capability to be enabled if needed. And also this change includes logic to exclude few flags when upload is disabled. ::: src/librygel-server/rygel-connection-manager-protocolinfo.vala @@ +41,3 @@ + private static ConnectionManagerProtocolInfo cm_protocol_info = null; + + internal class CMSProtocolInfo { Why can't you re-use the stuff from GUPnP-AV here? Couldn't we add this to GUPnP-AV instead? Feels like doubled effort. Response: gupnp-protocol-info in gupnp-av is tied to a protocolInfo value for a specific resource. The new CMSProtocolInfo class in Rygel is a helper for providing the latest sourceprotocolInfo value for the ConnectionManager service. This class has logic that aggregates the protocolInfo values of various resources available in the CDS and publish them(including non-protected and Link-protected content). Specs referenced : DLNA 7.3.28 section. ::: src/librygel-server/rygel-content-directory.vala @@ +471,3 @@ } + private async void get_all_items () { Why is this called get_all_items() when it updates the protocol_info? Response: Each time a content changes in CDS, SourceprotocolInfo has to be updated. get_all_items() is invoked to make sure we have the updated list of CDS items to create an Union of all protocolInfos.
Review of attachment 257836 [details] [review]: Just commenting on the playspeed things for now... It's a pretty daunting patch as a whole. ::: src/librygel-server/rygel-dlna-playspeed.vala @@ +42,3 @@ + * + */ +public class Rygel.DLNAPlaySpeedRequest : GLib.Object { This whole class could be just a new constructor for PlaySpeed (PlaySpeed.from_speed_request(Rygel.HTTPGet request)), right? @@ +108,3 @@ + * This class represents a DLNA PlaySpeed response (PlaySpeed.dlna.org) + */ +public class Rygel.DLNAPlaySpeedResponse : Rygel.HTTPResponseElement { Is this actually used as a HTTPResponseElement somewhere? If not it looks like Playspeed could just have a "add_response_header(int frame_rate = -1)" function in it? @@ +208,3 @@ + } + + private void parse(string speed) throws DLNAPlaySpeedError { mpris renderer plugin has play speed parsing, and my improved patch set for the byte seeking support (bug 710368) moves that to MediaPlayer. That code doesn't work the same though as it just returns a double. I'm not opposed to a class like this although in the renderer case the advantages are pretty minimal. I dislike the big API though: only one of is_negative/is_positive is needed (as 0 is not possible), likewise for is_normal_rate/is_trick_rate.
Review of attachment 257836 [details] [review]: ::: data/rygel.conf @@ +50,3 @@ +# Possible values - image-upload;av-upload;audio-upload +# This will take effect only if allow-upload is set to true +upload-option=image-upload;av-upload;audio-upload "upload-option" was just an addition to the existing allow-upload property, so placed it in the general section as the parent property "allow-upload"
Review of attachment 257836 [details] [review]: ::: src/librygel-core/rygel-description-file.vala @@ +212,2 @@ if (allow_upload) { + foreach (var option in upload_options) { Yes, This change allows granular upload capability to be enabled if needed. And also this change includes logic to exclude few flags when upload is disabled. ::: src/librygel-server/rygel-connection-manager-protocolinfo.vala @@ +41,3 @@ + private static ConnectionManagerProtocolInfo cm_protocol_info = null; + + internal class CMSProtocolInfo { gupnp-protocol-info in gupnp-av is tied to a protocolInfo value for a specific resource. The new CMSProtocolInfo class in Rygel is a helper for providing the latest sourceprotocolInfo value for the ConnectionManager service. This class has logic that aggregates the protocolInfo values of various resources available in the CDS and publish them(including non-protected and Link-protected content). Specs referenced : DLNA 7.3.28 section. ::: src/librygel-server/rygel-content-directory.vala @@ +471,3 @@ } + private async void get_all_items () { Each time a content changes in CDS, SourceprotocolInfo has to be updated. get_all_items() is invoked to make sure we have the updated list of CDS items to create an Union of all protocolInfos. ::: src/librygel-server/rygel-relational-expression.vala @@ +40,3 @@ internal const string CAPS = "@id,@parentID,@refID,upnp:class," + "dc:title,upnp:artist,upnp:album," + + "dc:creator,@childCount"; upnp:createClass search caps is added in rygel-content-directory only if upload functionality is enabled. 131 bool allow_upload = false; 132 try { 133 var config = MetaConfig.get_default (); 134 allow_upload = config.get_allow_upload (); 135 } catch (GLib.Error error) { } 136 137 if (allow_upload) { 138 this.search_caps += ",upnp:createClass"; 139 }
Review of attachment 257836 [details] [review]: ::: src/librygel-server/rygel-dlna-playspeed.vala @@ +42,3 @@ + * + */ +public class Rygel.DLNAPlaySpeedRequest : GLib.Object { Would then have to give some thought as to whether PlaySpeed would also be a HTTPResponseElement (since it also needs to grok playspeeds and we wouldn't want to duplicate the logic). @@ +108,3 @@ + * This class represents a DLNA PlaySpeed response (PlaySpeed.dlna.org) + */ +public class Rygel.DLNAPlaySpeedResponse : Rygel.HTTPResponseElement { Yeah, it definitely is used as a HTTPResponseElement (only OdidMediaEngine supports it, of course - see Bug 710704 for the code). I guess one could consider having this all be one class. There's just not a lot of overlap regarding their role, outside of groking fractional playspeeds. And if that's moved to its own class... also, would add_response_header() have anything in common with add_response_headers()? Or are you thinking along the lines of "add_framerate_response_header(int) that would just store away the framerate for add_response_headers(HTTPRequest)? @@ +134,3 @@ + base(); + this.speed = new DLNAPlaySpeed.from_string(speed); + this.framerate = NO_FRAMERATE; this is a bug. Should be "this.framerate = framerate;" @@ +208,3 @@ + } + + private void parse(string speed) throws DLNAPlaySpeedError { could we just move this to rygel core if we need to move this to a separate file anyway? (and maybe rename to just "PlaySpeed"?) We're using it in OdidMediaEngine in a few places. And could throw in a "to_double()" for wider use. I'm ok with removing is_negative()/is_trick_rate() (or is_positive()/is_normal_rate()) if the code remains sufficiently readable...
There is a new bug with an updated patch