Extend the Player API with the set_rate function

[georgeroman]

No description provided.

[ceyusa]

related with servo/servo#22293

[ceyusa]

keep in mind that some media cannot be played at different rates, and player will return an error via connect_error()

right now all the player's error are handled equally: the player stops. We might want something different. Perhaps we should check if there's an error right after the rate set. Or better yet, check the media info from player and verify if the stream is seekable, if it's seekable, then the rate change is possible.

[sdroege]

I would also suggest that based on your requirements we extend the player error API to actually give useful details.

[ferjm]

Thanks for the PR!

The code looks good, but I'd like us to apply @ceyusa's suggestion about checking if the stream is seekable before changing the playback rate.

We are already getting the stream's media info and converting it to our own metadata format. We will need to add an is_seekable there and set it according to the PlayerMediaInfo.is_seekable() result.

[ferjm]

I would also suggest that based on your requirements we extend the player error API to actually give useful details.

I filed #169 to track that work

[georgeroman]

Ok, so I added the requested changes but I have one question. What should be done in the case where last_metadata is not available? In this case how could I check if the media is seekable since last_metadata is None?
Currently, I'm just returning Ok(()) in this case:
https://github.com/servo/media/pull/168/files#diff-1d803d24b8ea49dbfeb827b92c7f9c28R153

[ferjm]

If last_metadata is not available is because the player has not started playing. In that case, we can store the value in the PlayerInner struct. And set the rate when we get the media info here if the stream is seekable.

We do something similar with input_size and stream_type.

[georgeroman]

I added the requested changes.

[ferjm]

And set the rate when we get the media info here if the stream is seekable.

I think you missed this last part. We still need to set the stored rate when we get the media info.

[georgeroman]

Yeah, it looks like I missed that. Now I modified the code accordingly.

[ferjm]

Thanks! The changes look good. However, this seems to be making the simple_player test panic. I am not sure why yet.

[georgeroman]

Shouldn't the rate change event be handled using connect_property_rate_notify?

[ferjm]

Sorry for the late reply here. Last week was a bit busy for many Mozillians :)

Shouldn't the rate change event be handled using connect_property_rate_notify?

Yes, it may be good to expose a Player event based on this signal, so we can trigger the ratechange event more accurately.

However, this seems to be making the simple_player test panic. I am not sure why yet.

The problem was that the test was buggy. I fixed it in #173, so rebasing on top of master should make this PR pass the tests as well.

[ferjm]

I don't think this is the right place to set the rate property. IIUC this event is triggered whenever the rate property changes, so we are effectively creating a loop here (unless GStreamer controls this somehow).

You are probably observing the simple_player test pass after this change because this event is never triggered (?).

I think the right place to set the rate is when we get the media info and we are able to check if the stream is seekable, as we were doing previously.

[georgeroman]

Yeah, it makes sense, I didn't fully understand what was happening there but I got it now. Ok, so I modified the rate update as it was previously done.

[ferjm]

One final change, let's add this comment here:

This method may be called before the player setup is done, so we safe the rate value and set it once the player is ready and after getting the media info

[ferjm]

Thanks!