GNOME Bugzilla – Bug 644538
Provide file extension for proxy/trancoding URIs
Last modified: 2012-01-19 18:50:20 UTC
Some devices (e.g TechniSat InternetRadio 1) require a file extension at the end of the URIs so we must provide that in our URIs.
Created attachment 200083 [details] [review] Adds extension to URIs of served files.
Created attachment 200084 [details] [review] Adds extension to URIs of served files.
Comment on attachment 200083 [details] [review] Adds extension to URIs of served files. Marking obsolete since it's the same as the other
Review of attachment 200084 [details] [review]: This also appends the extension for the original file to the thumbnail: <upnp:albumArtURI dlna:profileID="JPEG_TN">http://192.168.34.43:1234/MediaExport/i/NDQyYjAxYmY2OWViOTkwN2FiZWYwM2JkOTMwMzQ0YjU%3D/th/0.ogg</upnp:albumArtURI> ::: src/rygel/rygel-aac-transcoder.vala @@ +37,2 @@ public AACTranscoder () { + base ("audio/3gpp", "AAC_ISO_320", AudioItem.UPNP_CLASS, "aac"); The proper extension should be 3gp as that's the "file format" we're using here. ::: src/rygel/rygel-http-item-uri.vala @@ +48,3 @@ this.http_server = http_server; + + bool got_extension = false; You can use "var" here @@ +52,3 @@ + if (transcode_target != null) { + try { + this.extension = this.http_server.get_transcoder (transcode_target).extension; Line too long @@ +56,3 @@ + } catch (Error err) {} + } + if (!got_extension) { You can just skip the boolean flag and check if this.extension is null. @@ +139,2 @@ } else if (this.subtitle_index >= 0) { + path += "/sub/" + this.subtitle_index.to_string () + ".srt"; Wouldn't it be easier to put text/srt in the mime type list and just add the extension once? @@ +152,3 @@ } + + private string extension_to_append () { Doing this in a property might be more elegant @@ +160,3 @@ + + private string ext_from_mime_type (string mime_type) { + if (mime_to_ext.is_empty) { This might break for application/ogg, also there's no mkv. Also I wonder if we could use sth. like the reverse of G_FILE_ATTRIBUTE_STANDARD_FAST_CONTENT_TYPE so that we don't need to code the knowledge here at least for the non-DLNA file types (the mime types for DLNA sometimes differ from the "usual" ones"). ::: src/rygel/rygel-mp2ts-transcoder.vala @@ +48,2 @@ public MP2TSTranscoder (MP2TSProfile profile) { + base ("video/mpeg", PROFILES[profile], VideoItem.UPNP_CLASS, "mpeg"); Better use "mpg" here.
(In reply to comment #4) > Review of attachment 200084 [details] [review]: > > This also appends the extension for the original file to the thumbnail: > > <upnp:albumArtURI > dlna:profileID="JPEG_TN">http://192.168.34.43:1234/MediaExport/i/NDQyYjAxYmY2OWViOTkwN2FiZWYwM2JkOTMwMzQ0YjU%3D/th/0.ogg</upnp:albumArtURI> > I expected here some jpeg/png file being passed, not ogg. > ::: src/rygel/rygel-aac-transcoder.vala > @@ +37,2 @@ > public AACTranscoder () { > + base ("audio/3gpp", "AAC_ISO_320", AudioItem.UPNP_CLASS, "aac"); > > The proper extension should be 3gp as that's the "file format" we're using > here. Done. > ::: src/rygel/rygel-http-item-uri.vala > @@ +48,3 @@ > this.http_server = http_server; > + > + bool got_extension = false; > > You can use "var" here > > @@ +52,3 @@ > + if (transcode_target != null) { > + try { > + this.extension = this.http_server.get_transcoder > (transcode_target).extension; > > Line too long I splitted it into: var ts = this.http_server.get_transcoder (transcode_target); this.extension = ts.extension; > @@ +56,3 @@ > + } catch (Error err) {} > + } > + if (!got_extension) { > > You can just skip the boolean flag and check if this.extension is null. I thought if a field is "string" not "string?" then there is no sense in checking it against null. > > @@ +139,2 @@ > } else if (this.subtitle_index >= 0) { > + path += "/sub/" + this.subtitle_index.to_string () + ".srt"; > > Wouldn't it be easier to put text/srt in the mime type list and just add the > extension once? Sure, provided that a mime type of passed text will actually be text/srt. > @@ +152,3 @@ > } > + > + private string extension_to_append () { > > Doing this in a property might be more elegant Done. > @@ +160,3 @@ > + > + private string ext_from_mime_type (string mime_type) { > + if (mime_to_ext.is_empty) { > > This might break for application/ogg, also there's no mkv. Also I wonder if we > could use sth. like the reverse of G_FILE_ATTRIBUTE_STANDARD_FAST_CONTENT_TYPE > so that we don't need to code the knowledge here at least for the non-DLNA file > types (the mime types for DLNA sometimes differ from the "usual" ones"). I can't think of any such thing. You can get several extensions from one mime type. If this breaks for application/ogg then we have to manually add application/ogg and one of its corresponding extensions to the map. > > ::: src/rygel/rygel-mp2ts-transcoder.vala > @@ +48,2 @@ > public MP2TSTranscoder (MP2TSProfile profile) { > + base ("video/mpeg", PROFILES[profile], VideoItem.UPNP_CLASS, "mpeg"); > > Better use "mpg" here. Done.
(In reply to comment #5) > (In reply to comment #4) > > Review of attachment 200084 [details] [review] [details]: > > > > @@ +152,3 @@ > > } > > + > > + private string extension_to_append () { > > > > Doing this in a property might be more elegant > > Done. > Well, not done. Vala does not permit returning a new string from property getter with full ownership. rygel-http-item-uri.vala:37.17-37.39: error: Return value transfers ownership but method return type hasn't been declared to transfer ownership return "." + extension; ^^^^^^^^^^^^^^^^^^^^^^^
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > Review of attachment 200084 [details] [review] [details] [details]: > > > > > > @@ +152,3 @@ > > > } > > > + > > > + private string extension_to_append () { > > > > > > Doing this in a property might be more elegant > > > > Done. > > > > Well, not done. Vala does not permit returning a new string from property > getter with full ownership. > > rygel-http-item-uri.vala:37.17-37.39: error: Return value transfers ownership > but method return type hasn't been declared to transfer ownership > return "." + extension; > ^^^^^^^^^^^^^^^^^^^^^^^ It does, you just have to declare the getter as 'owned' IIRC.
Created attachment 200412 [details] [review] Adds extension to URIs of served files, 2. Code style fixed, rebased against current master. The issue with thumbnail extension is not yet addressed.
Created attachment 200415 [details] [review] Adds extension to URIs of served files, 3. Same as previous, but now has an owned property.
Review of attachment 200415 [details] [review]: Still doesn't work for thumbnails and subtitles since it uses the item's mime-type when creating the URI. ::: src/rygel/rygel-http-item-uri.vala @@ +45,3 @@ + } + + public static HashMap<string, string> mime_to_ext = new HashMap<string, string> (); still too long. You could also lazy-create it in ext_from_mime_type where you check for .empty () @@ +197,3 @@ + return mime_to_ext.get (mime_type); + } + return ""; Missing newline
(In reply to comment #5) > > @@ +56,3 @@ > > + } catch (Error err) {} > > + } > > + if (!got_extension) { > > > > You can just skip the boolean flag and check if this.extension is null. > > I thought if a field is "string" not "string?" then there is no sense in > checking it against null. Only in the experimental strict null mode. > > > > > @@ +139,2 @@ > > } else if (this.subtitle_index >= 0) { > > + path += "/sub/" + this.subtitle_index.to_string () + ".srt"; > > > > Wouldn't it be easier to put text/srt in the mime type list and just add the > > extension once? > > Sure, provided that a mime type of passed text will actually be text/srt. It won't. And same for thumbnails. G_FILE_ATTRIBUTE_STANDARD_FAST_CONTENT_TYPE > > so that we don't need to code the knowledge here at least for the non-DLNA file > > types (the mime types for DLNA sometimes differ from the "usual" ones"). > > I can't think of any such thing. You can get several extensions from one mime > type. If this breaks for application/ogg then we have to manually add > application/ogg and one of its corresponding extensions to the map. Yeah, I'm just a bit concerned of having to maintain that mapping.
(In reply to comment #10) > Review of attachment 200415 [details] [review]: > > ::: src/rygel/rygel-http-item-uri.vala > @@ +45,3 @@ > + } > + > + public static HashMap<string, string> mime_to_ext = new HashMap<string, > string> (); > > still too long. You could also lazy-create it in ext_from_mime_type where you > check for .empty () Done. > @@ +197,3 @@ > + return mime_to_ext.get (mime_type); > + } > + return ""; > > Missing newline Where? Between `}' and `return "";'? (In reply to comment #11) > (In reply to comment #5) > > > > @@ +56,3 @@ > > > + } catch (Error err) {} > > > + } > > > + if (!got_extension) { > > > > > > You can just skip the boolean flag and check if this.extension is null. > > > > I thought if a field is "string" not "string?" then there is no sense in > > checking it against null. > > Only in the experimental strict null mode. > Done. > > > > > > > > @@ +139,2 @@ > > > } else if (this.subtitle_index >= 0) { > > > + path += "/sub/" + this.subtitle_index.to_string () + ".srt"; > > > > > > Wouldn't it be easier to put text/srt in the mime type list and just add the > > > extension once? > > > > Sure, provided that a mime type of passed text will actually be text/srt. > > It won't. And same for thumbnails. So, the way to detect whether we have to generate URI for thumbnail/subtitles is to check thumbnail_index/subtitle_index whether it is greater than -1 in constructor? Are transcode_target, thumbnail_index and subtitle_index mutually exclusive (i.e. only one of them can have a different value from its default one)? > G_FILE_ATTRIBUTE_STANDARD_FAST_CONTENT_TYPE > > > so that we don't need to code the knowledge here at least for the non-DLNA file > > > types (the mime types for DLNA sometimes differ from the "usual" ones"). > > > > I can't think of any such thing. You can get several extensions from one mime > > type. If this breaks for application/ogg then we have to manually add > > application/ogg and one of its corresponding extensions to the map. > > Yeah, I'm just a bit concerned of having to maintain that mapping. Hopefully it won't that much of burden - probably there not that much devices that really need those extensions.
Created attachment 202068 [details] [review] Adds extension to URIs of served files, 4. New patch. I still don't know how thumbnails are generated, so for now I am checking if thumbnail index is greater than -1. If so, "png" will be an extension. Same with subtitles.
(In reply to comment #13) > Created an attachment (id=202068) [details] [review] > Adds extension to URIs of served files, 4. > > New patch. I still don't know how thumbnails are generated, so for now I am > checking if thumbnail index is greater than -1. If so, "png" will be an > extension. Same with subtitles. Hardcoding srt for now is fine, I suppose. For thumbnails it would probably be easier to just pass the item; a VisualItem will have an array of thumbnails attached where the thumbnail_index corresponds with the position in that array. So you could check the format of that thumbnail and select the proper extension.
Created attachment 203260 [details] [review] Adds extension to URIs of served files, 5. Ok, HTTPItemURI now takes MediaItem as a parameter - that allowed me to put most of extension guessing code into this class. I had to fix three tests as well. There is the same question repeated four times in HTTPItemURI's constructor as I didn't want to make this constructor to throw exceptions. I hope that the patch is mostly fine now.
The following fixes have been pushed: 674800e core: Append extensions to served files Minor editing done for style and adaptation to changed tests.
Created attachment 205653 [details] [review] core: Don't append extension on error
Created attachment 205654 [details] [review] core: Fix suffix for PNG thumbnails
Created attachment 205655 [details] [review] core: Append extensions to served files