GNOME Bugzilla – Bug 710368
Implement DLNA byte seeking
Last modified: 2013-11-11 08:02:02 UTC
Created attachment 257486 [details] [review] Patch, first throw Implements X_DLNAByteSeek operation
Review of attachment 257486 [details] [review]: Please check the whole code for tabs vs. spaces issues. ::: data/xml/AVTransport2.xml.in @@ +239,3 @@ + +<action> Indentation ::: src/librygel-renderer-gst/rygel-playbin-player.vala @@ +87,3 @@ + "video/x-ms-wmv", + "video/vnd.dlna.mpeg-tts", + }; This also needs modification of the supported DLNA profiles, no? @@ +148,3 @@ } + private string[] _allowed_playback_speeds = {"-64", "-32", "-16", "-8", "-4", "-2", "-1", "1/2", "1", "2", "4", "8", "10", "16", "32", "64"}; Long line, please reformat :) Also, if you update this string, you also need to modify the seek() implementation which is hard-coded to 1.0 to get the rate from MediaPlayer.playback_speed @@ +356,3 @@ + public bool seek_dlna (int64 target, string unit, double rate) { + Indenting, missing " " before curlies and ( ::: src/librygel-renderer/rygel-av-transport.vala @@ +133,3 @@ action_invoked["GetTransportInfo"].connect (this.get_transport_info_cb); action_invoked["GetPositionInfo"].connect (this.get_position_info_cb); +action_invoked["X_DLNA_GetBytePositionInfo"].connect (this.x_dlna_get_byte_position_info_cb); //siva_fix Indentation @@ +591,2 @@ this.player.playback_speed = speed; + this.changelog.log ("speed specified in play()", speed); the changelog object feeds the LastChange event and is not a common logging mechanism, please change to debug @@ +594,3 @@ + // Playback at 1x. + if (speed == "1") + { Curly after ) @@ +598,3 @@ + } + // Playback at more than 1x. Playspeed trick mode. + else Curly after else @@ +600,3 @@ + else + { + this.player.playback_state = "PLAYING"; the changelog object feeds the LastChange event and is not a common logging mechanism, please change to debug @@ +601,3 @@ + { + this.changelog.log ("Seeking to %s\n", this.player.position_as_str); + this.player.playback_state = "PLAYING"; playback_speed contains fraction strings (1/2) which double.parse can't handle; I suggest adding a helper method to TimeUtils or include the PS parsing stuff from one of the other patches if I've seen that correctly @@ +666,2 @@ return; + case "ABS_COUNT": Indentation @@ +669,3 @@ + case "X_DLNA_REL_BYTE": + debug ("Seeking bytes to %s.", target); + target.scanf("%lld", &count); Please use int64.parse() @@ +671,3 @@ + target.scanf("%lld", &count); + + if (!this.player.seek_dlna (count, "REL_COUNT", double.parse(this.player.playback_speed))) { Long line, please split, also missing " " before ( ::: src/librygel-renderer/rygel-media-player.vala @@ +112,3 @@ + * this many microseconds or bytes after the start. + */ + public abstract bool seek_dlna (int64 target, string unit, double rate); Why does this function have the additional rate parameter? It should just use the MediaPlayer.playback_speed value internally, no? ::: src/plugins/mpris/rygel-mpris-interfaces.vala @@ +51,3 @@ public abstract double volume { get; set; } public abstract int64 position { get; } + public abstract int64 position_byte { get; } This class maps the MPRIS2 DBus interface from http://mpris.org to a Vala class so these modifications are not necessary as these functions don't exist in MPRIS.
Also, what about X_DLNA_RelativeBytePosition, X_DLNA_AbsoluteBytePosition and X_DLNA_CurrentTrackSize?
this one seems to gone lost: double.parse can't handle fraction strings ("1/2") which can occur in player.playback_speed
Review of attachment 257486 [details] [review]: For AVTransport2.xml.in: allowedValueList should be removed from TransportPlaySpeed state variable (as vendor specific values are allowed and indeed the interesting ones). I can do that in a separate commit though. ::: src/librygel-renderer-gst/rygel-playbin-player.vala @@ +148,3 @@ } + private string[] _allowed_playback_speeds = {"-64", "-32", "-16", "-8", "-4", "-2", "-1", "1/2", "1", "2", "4", "8", "10", "16", "32", "64"}; I agree with the positive values: gstreamer seems to cope with those in video/mp4 just fine -- could even add some more fractions( "1/8","1/4"). The fact that less common formats won't be able to do these speeds shouldn't be a problem as the spec says 'best effort' is good enough when setting the speed. However, can gstreamer cope with negative speeds for any format with any reliability? If my experience is the norm (and gstreamer can't usually do negative speeds) we probably shouldn't advertize them. @@ +365,3 @@ + target * Gst.USECOND, + Gst.SeekType.NONE, + return this.playbin.seek (rate, I _think_ this will fail if rate is negative. I believe in that case it should be: this.playbin.seek (rate, Format.TIME, SeekFlags.FLUSH, Gst.SeekType.SET, 0, Gst.SeekType.SET, target * Gst.SECOND); although as I mentioned, I've never been able to test this reliably. ::: src/librygel-renderer/rygel-av-transport.vala @@ +601,3 @@ + { + this.changelog.log ("Seeking to %s\n", this.player.position_as_str); + this.player.playback_state = "PLAYING"; Christophe added rational_to_double/double_to_rational into rygel-mpris-player.vala at some point: should move those to a common file... ::: src/librygel-renderer/rygel-media-player.vala @@ +112,3 @@ + * this many microseconds or bytes after the start. + */ + public abstract bool seek_dlna (int64 target, string unit, double rate); I agree with Jens. In addition to that I wonder, shouldn't the ABS_TIME/REL_TIME case just call normal seek() instead if seek_dlna()? Then seek_dlna would degenerate into: public abstract bool seek_count (int64 count); or am I missing something?
Wow, I forgot how to bugzilla... Sorry for the bad quoting. The "this will fail if rate is negative" relates to seek_dlna() implementation in rygel-playbin-player.vala The rational_to_double comment relates to playback_speed parsing naturally.
Created attachment 257835 [details] [review] data: Remove allowedValueList from TransportPlaySpeed AVTransport2 TransportPlaySpeed state variable is allowed to have vendor specific values.
Also the new state variables are missing from the xml. Siva, unless you're already working on it, I'll have a go at cleaning this patch up.
Review of attachment 257486 [details] [review]: ::: src/librygel-renderer/rygel-av-transport.vala @@ -503,0 +513,16 @@ + private void x_dlna_get_byte_position_info_cb (Service service, + ServiceAction action) { + if (!this.check_instance_id (action)) { ... 13 more ... These are incorrect: RelByte and AbsByte are string types according to spec
Review of attachment 257486 [details] [review]: ::: src/librygel-renderer/rygel-av-transport.vala @@ -503,0 +513,16 @@ + private void x_dlna_get_byte_position_info_cb (Service service, + ServiceAction action) { + if (!this.check_instance_id (action)) { ... 13 more ... But we can do autoconversion and the type is the type of the passed value (it's a string in XML anyway, isn't it)
Review of attachment 257835 [details] [review]: Did you check whether this doesn't break DLNA cert? Shouldn't we rather generate this dynamically?
(In reply to comment #9) > But we can do autoconversion and the type is the type of the passed value > (it's a string in XML anyway, isn't it) I guess that's fine, it just looked fishy -- the type of the state variable in the xml should indeed be a string, if that was what you were asking. (In reply to comment #10) > Did you check whether this doesn't break DLNA cert? Shouldn't we rather > generate this dynamically? I did not and you may have a point. Is something like this done in any other place?
(In reply to comment #11) > (In reply to comment #9) > > But we can do autoconversion and the type is the type of the passed value > > (it's a string in XML anyway, isn't it) > > I guess that's fine, it just looked fishy -- the type of the state variable in > the xml should indeed be a string, if that was what you were asking. Well what I meant to say way that there's no type annotation in the XML anyway. > (In reply to comment #10) > > Did you check whether this doesn't break DLNA cert? Shouldn't we rather > > generate this dynamically? > > I did not and you may have a point. Is something like this done in any other > place? no, we usually hard-code the vendor-definable values (such as volume) and then translate to the backend accordingly. But we can't really do this here, I'm afraid. But maybe it doesn't matter :)
Jens / Jussi, My understanding from today's meeting was that, you will be addressing the comments and will provide me the updates for validation. Please let me know otherwise, if you are waiting on me to work on this comments or feedback. Thanks..
(In reply to comment #13) > My understanding from today's meeting was that, you will be addressing the > comments and will provide me the updates for validation. Please let me know > otherwise, if you are waiting on me to work on this comments or feedback. I'm working on the patch, will upload it here. ETA is monday. Please comment if any of our earlier comments seem somehow wrong though. Note on the the missing state variable discussion: I noticed in special circumstances the values can be empty string (as well as uint-disguised-as-string)...
While generating the TransportPlaySpeed allowedValueList would be best, my patch is technically correct as well: 7.4.1.6.20.1: ... If a UPnP AV MediaRenderer chooses not to specify a list of allowed play speed values, then AVT.TransportPlaySpeed will be defined in the service description document as follows: <stateVariable sendEvents="no"> <name>TransportPlaySpeed</name> <dataType>string</dataType> </stateVariable>
(In reply to comment #1) > Review of attachment 257486 [details] [review]: > > Please check the whole code for tabs vs. spaces issues. > > ::: data/xml/AVTransport2.xml.in > @@ +239,3 @@ > > + > +<action> > > Indentation > > ::: src/librygel-renderer-gst/rygel-playbin-player.vala > @@ +87,3 @@ > + "video/x-ms-wmv", > + "video/vnd.dlna.mpeg-tts", > + }; > > This also needs modification of the supported DLNA profiles, no? > > @@ +148,3 @@ > } > > + private string[] _allowed_playback_speeds = {"-64", "-32", "-16", "-8", > "-4", "-2", "-1", "1/2", "1", "2", "4", "8", "10", "16", "32", "64"}; > > Long line, please reformat :) > > Also, if you update this string, you also need to modify the seek() > implementation which is hard-coded to 1.0 to get the rate from > MediaPlayer.playback_speed Since I have introduced seek_dlna() new api to support both time and byte units, i did not modify the seek() implementation for back ward compatibility. But I agree that changing hard coded value 1.0 to the actual rate would not break any existing seek() implementation. > > @@ +356,3 @@ > > + public bool seek_dlna (int64 target, string unit, double rate) { > + > > Indenting, missing " " before curlies and ( > > ::: src/librygel-renderer/rygel-av-transport.vala > @@ +133,3 @@ > action_invoked["GetTransportInfo"].connect > (this.get_transport_info_cb); > action_invoked["GetPositionInfo"].connect (this.get_position_info_cb); > +action_invoked["X_DLNA_GetBytePositionInfo"].connect > (this.x_dlna_get_byte_position_info_cb); //siva_fix > > Indentation > > @@ +591,2 @@ > this.player.playback_speed = speed; > + this.changelog.log ("speed specified in play()", speed); > > the changelog object feeds the LastChange event and is not a common logging > mechanism, please change to debug > > @@ +594,3 @@ > + // Playback at 1x. > + if (speed == "1") > + { > > Curly after ) > > @@ +598,3 @@ > + } > + // Playback at more than 1x. Playspeed trick mode. > + else > > Curly after else > > @@ +600,3 @@ > + else > + { > + this.player.playback_state = "PLAYING"; > > the changelog object feeds the LastChange event and is not a common logging > mechanism, please change to debug > > @@ +601,3 @@ > + { > + this.changelog.log ("Seeking to %s\n", > this.player.position_as_str); > + this.player.playback_state = "PLAYING"; > > playback_speed contains fraction strings (1/2) which double.parse can't handle; > I suggest adding a helper method to TimeUtils or include the PS parsing stuff > from one of the other patches if I've seen that correctly Agree with you on the double.parse() using string "1/2". > > @@ +666,2 @@ > return; > + case "ABS_COUNT": > > Indentation > > @@ +669,3 @@ > + case "X_DLNA_REL_BYTE": > + debug ("Seeking bytes to %s.", target); > + target.scanf("%lld", &count); > > Please use int64.parse() I am ok using int64.parse() instead of string.scanf() > > @@ +671,3 @@ > + target.scanf("%lld", &count); > + > + if (!this.player.seek_dlna (count, "REL_COUNT", > double.parse(this.player.playback_speed))) { > > Long line, please split, also missing " " before ( > > ::: src/librygel-renderer/rygel-media-player.vala > @@ +112,3 @@ > + * this many microseconds or bytes after the start. > + */ > + public abstract bool seek_dlna (int64 target, string unit, double rate); > > Why does this function have the additional rate parameter? It should just use > the MediaPlayer.playback_speed value internally, no? New additional rate parameter can be used to set after seeking. For example if you want to seek and start playing back at "rate" speed. But your question is valid, in a typical graphical slide bar user interface it is not possible to set different speed after the seek. > > ::: src/plugins/mpris/rygel-mpris-interfaces.vala > @@ +51,3 @@ > public abstract double volume { get; set; } > public abstract int64 position { get; } > + public abstract int64 position_byte { get; } > > This class maps the MPRIS2 DBus interface from http://mpris.org to a Vala class > so these modifications are not necessary as these functions don't exist in > MPRIS. I think I was getting build error without these modifications.
In general, thanks for finding minor issues on syntax, debugs and spacing. I could have cleaned up before submitting the patch to you.
Ok, I'm uploading the patch even though I have problems getting playbin to work really reliably... I think the basics are a lot cleaner now but it is quite different from original patch so I would like feedback. For playbin, currently enabled playspeeds work quite nicely here (mp4) and seeking to absolute byte positions works reliably as well. Remaining problems: * negative playspeeds do not seem to work on playbin: Could be my mistake or could be that this is not supported on any media format. This is annoying but does not prevent patch from landing as we can limit allowed playspeeds (and they are "best effort" anyway) * Querying byte position seems to work (returns seemingly right values) but prints this: GStreamer-CRITICAL **: gst_query_set_position: assertion 'format == g_value_get_enum (gst_structure_id_get_value (s, GST_QUARK (FORMAT)))' failed You can see this whenever you do a relative byte seek * relative byte seeking is _very_ unreliable: seeking 10M bytes forward for 4-5 times is enough to end up in a totally wrong place. I'm assuming it is related to the above problem. I am investigating but no results so far :( * Currently a Seek() won't reset playspeed to "1". Not sure if it should Changes I've done: * xml: Added seekmode allowedValues ABS_COUNT, REL_COUNT and X_DLNA_REL_BYTE. Not sure if I'm dense or what but I haven't found a good explanation of what exactly these are supposed to mean -- they now operate like time seek: ABS_COUNT is bytes from beginning of track and the other two relative to current byte position * xml: Added needed statevariables * removed non-functional comments "modification made by" -- this info is stored in git, no need to write it out. Moved copyright notices and authorship notices to right place. * Removed the new methods from MPRIS.MediaPlayerProxy. We can't just go adding things to an API we don't implement ourselves! * added MediaPlayer::can_seek_bytes(). Added MediaPlayer::play_speed_to_double() helper function renamed MediaPlayer::position_byte -> MediaPlayer::byte_position, renamed MediaPlayer::seek_dlna -> MediaPlayer::seek_bytes and modified it to have a simpler signature: it now uses internal play speed * Added missing "X_DLNA_SeekByte" to TransportActions * AVTransport usedto call seek_dlna() from "Play()" call... removed that as it seemed hacky: now it just sets speed and playback_state * AVTransport::x_dlna_get_byte_position_info_cb now handles the variables as string: it needs to be done as "" is sometimes returned * Changed the playbin implementation to only seek for play speed change after playbin is playing * playbin allowed speeds: Added fractional play speeds, removed negative playspeeds as they don't seem to work. Hints on this would be welcome * simplified the playbin byte seek implementation
Created attachment 258568 [details] [review] renderer: Implement DLNA byte seeking
Created attachment 258569 [details] [review] renderer: Add play_speed_to_double() to MediaPlayer
Created attachment 258570 [details] [review] renderer: Implement DLNA byte seeking
Created attachment 258571 [details] [review] renderer-gst: Enable more playspeeds
Review of attachment 258570 [details] [review]: ::: src/librygel-renderer-gst/rygel-playbin-player.vala @@ +359,3 @@ + seeked = this.playbin.seek (speed, + format, + SeekFlags.FLUSH | SeekFlags.SKIP | SeekFlags.ACCURATE, The seek flags are something I'd be happy to get recommendations on -- what to use or not to use.
Review of attachment 258570 [details] [review]: ::: src/librygel-renderer-gst/rygel-playbin-player.vala @@ +312,3 @@ + int64 pos; + + if (this.playbin.query_position (Format.BYTES, out pos)) { Hmm, so this is apparently wrong: "if you query a pipeline for it in BYTES, it will be passed to the sinks, and then you should theoretically get the byte position in the raw media stream for whatever sink answers first, which is not what you want" I'll have to try a source element instead I guess?
I can't claim to fully understand the gstreamer magic but this.playbin.source.query_position (Format.BYTES, out pos) does seem to work: I can do dozens of back and forth byte seeks and the results seem logical to me. I still need to add MediaPlayer.byte_duration or MediaPlayer.byte_length property so Seek() can return 711 if seeking outside the byte length just like it does for time seeks.
* negative playspeeds do not seem to work on playbin: Could be my mistake or could be that this is not supported on any media format. This is annoying but does not prevent patch from landing as we can limit allowed playspeeds (and they are "best effort" anyway) >>>At this time negative playspeeds does not supported by Gstreamer playbin, for any media formats. It is a known issue. * Querying byte position seems to work (returns seemingly right values) but prints this: GStreamer-CRITICAL **: gst_query_set_position: assertion 'format == g_value_get_enum (gst_structure_id_get_value (s, GST_QUARK (FORMAT)))' failed You can see this whenever you do a relative byte seek >>>I can check with our Gstreamer team about this issue and your other other finding related to gst_query_set_position()
Created attachment 258642 [details] [review] renderer: Implement DLNA byte seeking This change starts using the playbin source element for byte queries: this seems to work and the gstreamer people I talked to didn't seem to think this solution was wrong. It also adds "byte_duration" property to MediaPlayer.
(In reply to comment #22) > Created an attachment (id=258571) [details] [review] > renderer-gst: Enable more playspeeds I can't seem to apply this patch using git bz: fatal: sha1 information is lacking or useless (src/librygel-renderer-gst/rygel-playbin-player.vala).
Also, the patch doesn't apply manually, it seems something missing here...
sorry, my bad, had to change the patch order.
Review of attachment 258642 [details] [review]: I think it's fine in general ::: src/librygel-renderer-gst/rygel-playbin-player.vala @@ +86,3 @@ "video/x-xvid", + "video/x-ms-wmv", + "video/vnd.dlna.mpeg-tts"}; Unrelated change, also needs addition of MPEG_PS_* stuff to DLNA profiles IMHO. ::: src/librygel-renderer/rygel-av-transport.vala @@ +567,2 @@ this.player.playback_speed = speed; + this.player.playback_state = "PLAYING"; Can you add a comment about the necessity of this order, please? I had to think twice why this change is necessary so I think I'm not the only one who'd likely break it later on. @@ +605,2 @@ var seek_target = TimeUtils.time_from_string (target); + if (unit != "ABS_TIME") { Curios change - why that? ::: src/librygel-renderer/rygel-media-player.vala @@ +55,3 @@ + /// Duration of the current media in bytes + public abstract int64 byte_duration { get; } Why not size? ::: src/plugins/mpris/rygel-mpris-interfaces.vala @@ +52,3 @@ public abstract void play () throws DBusError; public abstract void seek (int64 offset) throws DBusError; + public abstract void seek_bytes (int64 offset) throws DBusError; Not necessary, as in original patch that's for mapping the MPRIS DBus interface to vala. Works fine without.
Review of attachment 258569 [details] [review]: Fine apart from that style issue ::: src/librygel-renderer/rygel-media-player.vala @@ +112,3 @@ + protected double play_speed_to_double (string speed) + { + string[] rational = speed.split("/", 2); Missing " " before (
Review of attachment 258571 [details] [review]: Apart from minor things ok ::: src/librygel-renderer-gst/rygel-playbin-player.vala @@ +162,3 @@ } + private string _new_playback_speed = "1"; Please add a comment what _new_playback_speed is needed for
Created attachment 258993 [details] [review] renderer: Add play_speed_to_double() to MediaPlayer This can be useful for all player implementations.
Created attachment 258994 [details] [review] renderer: Implement DLNA byte seeking Adds X_DLNA_GetBytePositionInfo() method, the required state variables and the missing SeekModes for Seek() to AVTransport2 implementation.
Created attachment 258995 [details] [review] renderer-gst: Enable more playspeeds Negative playspeeds do not seem to work with common formats, so they have not been enabled.
Thanks for review. (In reply to comment #32) > ::: src/librygel-renderer-gst/rygel-playbin-player.vala > @@ +86,3 @@ > "video/x-xvid", > + "video/x-ms-wmv", > + "video/vnd.dlna.mpeg-tts"}; > > Unrelated change, also needs addition of MPEG_PS_* stuff to DLNA profiles IMHO. Removed. > ::: src/librygel-renderer/rygel-av-transport.vala > @@ +567,2 @@ > this.player.playback_speed = speed; > + this.player.playback_state = "PLAYING"; > > Can you add a comment about the necessity of this order, please? I had to think > twice why this change is necessary so I think I'm not the only one who'd likely > break it later on. Got it. > @@ +605,2 @@ > var seek_target = TimeUtils.time_from_string (target); > + if (unit != "ABS_TIME") { > > Curios change - why that? Just so the code for time and byte seeks is as similar as possible (the check is inverted because on byte seek side the other way would require checking both X_DLNA_REL_BYTE and REL_COUNT). > ::: src/librygel-renderer/rygel-media-player.vala > @@ +55,3 @@ > > + /// Duration of the current media in bytes > + public abstract int64 byte_duration { get; } > > Why not size? For completeness with seek vs seek_bytes and position vs byte_position ... but I have to admit it sounds quite wrong to me as well. Changed to size. > ::: src/plugins/mpris/rygel-mpris-interfaces.vala > @@ +52,3 @@ > public abstract void play () throws DBusError; > public abstract void seek (int64 offset) throws DBusError; > + public abstract void seek_bytes (int64 offset) throws DBusError; > > Not necessary, as in original patch that's for mapping the MPRIS DBus interface > to vala. Works fine without. That was not supposed to be there, thanks. Also fixed the minor issues with the other two files.
When I was validating the attached patches, I have encountered an error in byte-seek. "Operation failed: Illegal seek target" I think the bytes should be checked against the this.size instead of this.duration. I have verified with the following fix. public bool seek_bytes (int64 bytes) { debug ("Seeking %lld bytes, play speed %s this.duration %lld this.size %lld", bytes, this._new_playback_speed, this.duration, this.size ); if (this.duration > 0 && bytes > this.size) { //siva_fix return false; } return this.seek_with_format (Format.BYTES, bytes); }
(In reply to comment #39) > I think the bytes should be checked against the this.size instead of > this.duration. Thanks Siva, you're right (although the "> 0" check needs to be against size as well). I'll re-upload.
Created attachment 259076 [details] [review] renderer: Implement DLNA byte seeking Adds X_DLNA_GetBytePositionInfo() method, the required state variables and the missing SeekModes for Seek() to AVTransport2 implementation.
Created attachment 259077 [details] [review] renderer-gst: Enable more playspeeds Negative playspeeds do not seem to work with common formats, so they have not been enabled.
Review of attachment 257835 [details] [review]: If removing doesn't break CTT, go for it
Review of attachment 258993 [details] [review]: Looks good apart from that minor comment ::: src/librygel-renderer/rygel-media-player.vala @@ +114,3 @@ + string[] rational = speed.split ("/", 2); + + */ Doesn't that - by defintion - mean that the whole fraction is just "0"? I know that this shouldn't happen in our context, but that's valid.
Review of attachment 259077 [details] [review]: Looks good
Review of attachment 259076 [details] [review]: Go ahead ::: src/librygel-renderer-gst/rygel-playbin-player.vala @@ +299,3 @@ + int64 dur; + + if (this.playbin.source.query_duration (Format.BYTES, out dur)) { oh, playbin has a "source" property, interesting.
Attachment 257835 [details] pushed as 610f358 - data: Remove allowedValueList from TransportPlaySpeed Attachment 258993 [details] pushed as d9e5244 - renderer: Add play_speed_to_double() to MediaPlayer Attachment 259076 [details] pushed as 0347a67 - renderer: Implement DLNA byte seeking Attachment 259077 [details] pushed as 8cab0ca - renderer-gst: Enable more playspeeds
All these patches are pushed into "master" branch?
(In reply to comment #48) > All these patches are pushed into "master" branch? That's correct. Thanks a lot Jens.