companion "precise" skip implementation

[thiccaxe]

implements the skip-by-behavior found in the companion protocol. I don't know if #2363 is going to get merged or not. Additionally, this skip feature is much more precise and thus utile than fast forward or rewind anyway.

[postlund]

I believe we should implement the skip_forward and skip_backward methods instead of adding a new API method for such a small edge case. Every new API method potentially means additional work in the other protocols as well (to be more "feature complete"). Question is if iOS uses a hard coded "default time" or if it gets that time from the app/active player. This is the case for MRP, where apps can use arbitrary times for skipping. Maybe we can verify this with the proxy?

[thiccaxe]

on companion devices TVRC uses a hard coded default time of 10 seconds from what I can gather.

for such a small edge case

ios itself uses this feature to go to the beginning of a title (albeit as somewhat of a hack, setting the _skpS to -999999.9 or so), so this is needed in some way or another, at the very least (better to expose this "hack" straight to the developer as opposed to covering it up in pyatv...)

I agree with your sentiment, though. I considered augmenting skip_forward/backward with a new parameter with the default set to the 10 seconds found in TVRC. However, it is probably worse to change the old api...

to be more "feature complete"

So now I looked deeper into what dmap and mrp do.

Doesn't sound like much more effort, dmap and mrp only need a small amount of code to support this?
Anyway, i see that mrp has skip_interval = 15 # Default value. More over, dmap has a different default skip time of 10 seconds. These probably come from TVRC and common app values.

If implemented as you say, the methods skip_forward/skip_backward would be wrappers on all three protocols, limiting their use overall. I don't see how this is beneficial.
I agree that skip_forwards and skip_backwards should be added to companion. However, it makes sense to continue and add the precise skip to all three protocols anyway.

However, I cannot test mrp, and probably cannot test dmap.

[postlund]

on companion devices TVRC uses a hard coded default time of 10 seconds from what I can gather.

for such a small edge case

ios itself uses this feature to go to the beginning of a title (albeit as somewhat of a hack, setting the _skpS to -999999.9 or so), so this is needed in some way or another, at the very least (better to expose this "hack" straight to the developer as opposed to covering it up in pyatv...)

I agree with your sentiment, though. I considered augmenting skip_forward/backward with a new parameter with the default set to the 10 seconds found in TVRC. However, it is probably worse to change the old api...

to be more "feature complete"

So now I looked deeper into what dmap and mrp do.

Doesn't sound like much more effort, dmap and mrp only need a small amount of code to support this? Anyway, i see that mrp has skip_interval = 15 # Default value. More over, dmap has a different default skip time of 10 seconds. These probably come from TVRC and common app values.

If implemented as you say, the methods skip_forward/skip_backward would be wrappers on all three protocols, limiting their use overall. I don't see how this is beneficial. I agree that skip_forwards and skip_backwards should be added to companion. However, it makes sense to continue and add the precise skip to all three protocols anyway.

However, I cannot test mrp, and probably cannot test dmap.

Skip interval is probably not consistent in all protocols, should be fixed if possible and not implemented. My suggestion is to add parameter to skip_forward and skip_backward, e.g. interval and set it to 0 by default. Internally, each protocol should use the default value (e.g. 15s) if no argument is given/is set to zero. Otherwise use the provided value, if possible. You can then simulate the same behavior as iOS by giving a large value as argument to either of them.

[postlund]

Adding a new parameter with default argument is also backwards compatible.

[thiccaxe]

Ah, ok, this makes more sense now. I'll also update to factor in the latest merge.

[postlund]

@thiccaxe Any more changes incoming or ready for review?

[thiccaxe]

Should be good for review, I'm going to test a few more unusual numbers such as -infinity, NaN, see what happens on a real companion devices.

Also, I learned that opack can't encode negative integers (atleast small negative integers) - later, I'm going to look through the companion code base to make sure there isn't an odd case where that happens.

[postlund]

Should be good for review, I'm going to test a few more unusual numbers such as -infinity, NaN, see what happens on a real companion devices.

I will try to review it as soon as I can.

Also, I learned that opack can't encode negative integers (atleast small negative integers) - later, I'm going to look through the companion code base to make sure there isn't an odd case where that happens.

OPACK supports negative integers ("signed"). But you might have discovered a bug indeed. The integer range 0-39 can be encoded using a single byte (-1 has a special value as well). I'm not sure negative values are handled properly for this range. Might be worth adding some unit tests and verify.

[postlund]

You have some linting to fix (./scripts/chickn.py -t fixup).

[postlund]

Minor comment you can fix if you want, but looks good otherwise 👍

[postlund]

Just want to double check that you are done with this?

[thiccaxe]

Just want to double check that you are done with this?

yeah, should be good. Putting it down here as a note, since these are really far out edge cases:
skipbackward/forwards +- infinity goes to the start or end of the current media, and NaN does 10 seconds.