Repository: postlund/pyatv
Pull request #2462 (2024-07-30 07:03:05)

companion "precise" skip implementation
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: 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. 



Patch:
diff --git a/pyatv/core/facade.py b/pyatv/core/facade.py
index c1740943..4be00cca 100644
--- a/pyatv/core/facade.py
+++ b/pyatv/core/facade.py
@@ -146,20 +146,22 @@ class FacadeRemoteControl(Relayer, interface.RemoteControl):
         return await self.relay("wakeup")()
 
     @shield.guard
-    async def skip_forward(self) -> None:
+    async def skip_forward(self, time_interval: float = 0.0) -> None:
         """Skip forward a time interval.
 
-        Skip interval is typically 15-30s, but is decided by the app.
+        If time_interval is not positive or not present, a default or app-chosen
+        time interval is used, which is typically 10, 15, 30, etc. seconds.
         """
-        return await self.relay("skip_forward")()
+        return await self.relay("skip_forward")(time_interval)
 
     @shield.guard
-    async def skip_backward(self) -> None:
-        """Skip backwards a time interval.
+    async def skip_backward(self, time_interval: float = 0.0) -> None:
+        """Skip backward a time interval.
 
-        Skip interval is typically 15-30s, but is decided by the app.
+        If time_interval is not positive or not present, a default or app-chosen
+        time interval is used, which is typically 10, 15, 30, etc. seconds.
         """
-        return await self.relay("skip_backward")()
+        return await self.relay("skip_backward")(time_interval)
 
     @shield.guard
     async def set_position(self, pos: int) -> None:
diff --git a/pyatv/interface.py b/pyatv/interface.py
index 8a8d56f0..336bf4a1 100644
--- a/pyatv/interface.py
+++ b/pyatv/interface.py
@@ -407,18 +407,20 @@ class RemoteControl:
         "SkipForward",
         "Skip forward a time interval.",
     )
-    async def skip_forward(self) -> None:
+    async def skip_forward(self, time_interval: float = 0.0) -> None:
         """Skip forward a time interval.
 
-        Skip interval is typically 15-30s, but is decided by the app.
+        If time_interval is not positive or not present, a default or app-chosen
+        time interval is used, which is typically 10, 15, 30, etc. seconds.
         """
         raise exceptions.NotSupportedError()
 
     @feature(37, "SkipBackward", "Skip backwards a time interval.")
-    async def skip_backward(self) -> None:
-        """Skip backwards a time interval.
+    async def skip_backward(self, time_interval: float = 0.0) -> None:
+        """Skip backward a time interval.
 
-        Skip interval is typically 15-30s, but is decided by the app.
+        If time_interval is not positive or not present, a default or app-chosen
+        time interval is used, which is typically 10, 15, 30, etc. seconds.
         """
         raise exceptions.NotSupportedError()
 
diff --git a/pyatv/protocols/companion/__init__.py b/pyatv/protocols/companion/__init__.py
index dcd621ab..7d48dda7 100644
--- a/pyatv/protocols/companion/__init__.py
+++ b/pyatv/protocols/companion/__init__.py
@@ -77,6 +77,9 @@ PAIRING_DISABLED_MASK = 0x04
 # that pyatv supports).
 PAIRING_WITH_PIN_SUPPORTED_MASK = 0x4000
 
+# As seen in the TV Remote App
+_DEFAULT_SKIP_TIME = 10
+
 # pylint: disable=invalid-name
 
 
@@ -339,6 +342,29 @@ class CompanionRemoteControl(RemoteControl):
         """Press key previous."""
         await self.api.mediacontrol_command(MediaControlCommand.PreviousTrack)
 
+    async def skip_forward(self, time_interval: float = 0.0) -> None:
+        """Skip forward a time interval."""
+        await self.api.mediacontrol_command(
+            MediaControlCommand.SkipBy,
+            {
+                "_skpS": float(
+                    time_interval if time_interval > 0 else _DEFAULT_SKIP_TIME
+                )
+            },
+        )
+
+    async def skip_backward(self, time_interval: float = 0.0) -> None:
+        """Skip forward a time interval."""
+        # float cast: opack fails with negative integers
+        await self.api.mediacontrol_command(
+            MediaControlCommand.SkipBy,
+            {
+                "_skpS": float(
+                    -time_interval if time_interval > 0 else -_DEFAULT_SKIP_TIME
+                )
+            },
+        )
+
     async def channel_up(self) -> None:
         """Select next channel."""
         await self._press_button(HidCommand.ChannelIncrement)
diff --git a/pyatv/protocols/dmap/__init__.py b/pyatv/protocols/dmap/__init__.py
index ca15e702..662a975b 100644
--- a/pyatv/protocols/dmap/__init__.py
+++ b/pyatv/protocols/dmap/__init__.py
@@ -353,23 +353,29 @@ class DmapRemoteControl(RemoteControl):
         """Press key volume down."""
         await self.apple_tv.ctrl_int_cmd("volumedown")
 
-    async def skip_forward(self) -> None:
+    async def skip_forward(self, time_interval: float = 0.0) -> None:
         """Skip forward a time interval.
 
         Skip interval is typically 15-30s, but is decided by the app.
         """
         current_position = (await self.apple_tv.playstatus()).position
         if current_position:
-            await self.set_position(current_position + _DEFAULT_SKIP_TIME)
+            await self.set_position(
+                current_position
+                + (int(time_interval) if time_interval > 0 else _DEFAULT_SKIP_TIME)
+            )
 
-    async def skip_backward(self) -> None:
+    async def skip_backward(self, time_interval: float = 0.0) -> None:
         """Skip backwards a time interval.
 
         Skip interval is typically 15-30s, but is decided by the app.
         """
         current_position = (await self.apple_tv.playstatus()).position
         if current_position:
-            await self.set_position(current_position - _DEFAULT_SKIP_TIME)
+            await self.set_position(
+                current_position
+                - (int(time_interval) if time_interval > 0 else _DEFAULT_SKIP_TIME)
+            )
 
     async def set_position(self, pos: int) -> None:
         """Seek in the current playing media."""
diff --git a/pyatv/protocols/mrp/__init__.py b/pyatv/protocols/mrp/__init__.py
index 4323e976..685d2dea 100644
--- a/pyatv/protocols/mrp/__init__.py
+++ b/pyatv/protocols/mrp/__init__.py
@@ -71,6 +71,8 @@ from pyatv.support.url import is_url
 
 _LOGGER = logging.getLogger(__name__)
 
+_DEFAULT_SKIP_TIME = 15
+
 # Source: https://github.com/Daij-Djan/DDHidLib/blob/master/usb_hid_usages.txt
 _KEY_LOOKUP = {
     # name: [usage_page, usage]
@@ -429,28 +431,31 @@ class MrpRemoteControl(RemoteControl):
         """Wake up the device."""
         await _send_hid_key(self.protocol, "wakeup", InputAction.SingleTap)
 
-    async def skip_forward(self) -> None:
+    async def skip_forward(self, time_interval: float = 0.0) -> None:
         """Skip forward a time interval.
 
         Skip interval is typically 15-30s, but is decided by the app.
         """
-        await self._skip_command(CommandInfo_pb2.SkipForward)
+        await self._skip_command(CommandInfo_pb2.SkipForward, time_interval)
 
-    async def skip_backward(self) -> None:
+    async def skip_backward(self, time_interval: float = 0.0) -> None:
         """Skip backwards a time interval.
 
         Skip interval is typically 15-30s, but is decided by the app.
         """
-        await self._skip_command(CommandInfo_pb2.SkipBackward)
+        await self._skip_command(CommandInfo_pb2.SkipBackward, time_interval)
 
-    async def _skip_command(self, command) -> None:
+    async def _skip_command(self, command, time_interval: float) -> None:
         info = self.psm.playing.command_info(command)
 
+        skip_interval: int
+        if time_interval > 0:
+            skip_interval = int(time_interval)
         # Pick the first preferred interval for simplicity
-        if info and info.preferredIntervals:
+        elif info and info.preferredIntervals:
             skip_interval = info.preferredIntervals[0]
         else:
-            skip_interval = 15  # Default value
+            skip_interval = _DEFAULT_SKIP_TIME  # Default value
 
         await self._send_command(command, skipInterval=skip_interval)
 


Test patch:
diff --git a/tests/fake_device/companion.py b/tests/fake_device/companion.py
index f910e9f6..fe6fda05 100644
--- a/tests/fake_device/companion.py
+++ b/tests/fake_device/companion.py
@@ -23,6 +23,7 @@ _LOGGER = logging.getLogger(__name__)
 
 DEVICE_NAME = "Fake Companion ATV"
 INITIAL_VOLUME = 10.0
+INITIAL_DURATION = 10.0
 VOLUME_STEP = 5.0
 INITIAL_RTI_TEXT = "Fake Companion Keyboard Text"
 
@@ -55,6 +56,7 @@ MEDIA_CONTROL_MAP = {
     MediaControlCommand.NextTrack: "next",
     MediaControlCommand.PreviousTrack: "previous",
     MediaControlCommand.SetVolume: "set_volume",
+    MediaControlCommand.SkipBy: "skip",
 }
 
 
@@ -97,6 +99,7 @@ class FakeCompanionState:
         self.media_control_flags: int = MediaControlFlags.Volume
         self.interests: Set[str] = set()
         self.volume: float = INITIAL_VOLUME
+        self.duration: float = INITIAL_DURATION
         self.rti_clients: List[FakeCompanionService] = []
         self._rti_focus_state: KeyboardFocusState = KeyboardFocusState.Focused
         self.rti_text: Optional[str] = INITIAL_RTI_TEXT
@@ -454,8 +457,10 @@ class FakeCompanionService(CompanionServerAuth, asyncio.Protocol):
         if mcc == MediaControlCommand.SetVolume:
             # Make sure we send response before triggering event with volume update
             self.loop.call_soon(self.volume_changed(message["_c"]["_vol"] * 100.0))
-        if mcc == MediaControlCommand.GetVolume:
+        elif mcc == MediaControlCommand.GetVolume:
             args["_vol"] = self.state.volume / 100.0
+        elif mcc == MediaControlCommand.SkipBy:
+            self.state.duration = max(0, self.state.duration + message["_c"]["_skpS"])
         elif mcc in MEDIA_CONTROL_MAP:
             _LOGGER.debug("Activated Media Control Command %s", mcc)
             self.state.latest_button = MEDIA_CONTROL_MAP[mcc]
diff --git a/tests/protocols/companion/test_companion_functional.py b/tests/protocols/companion/test_companion_functional.py
index 796c0c5e..e4c55c63 100644
--- a/tests/protocols/companion/test_companion_functional.py
+++ b/tests/protocols/companion/test_companion_functional.py
@@ -20,6 +20,7 @@ from pyatv.interface import App, FeatureName, FeatureState, UserAccount
 from pyatv.protocols.companion.api import SystemStatus
 
 from tests.fake_device.companion import (
+    INITIAL_DURATION,
     INITIAL_RTI_TEXT,
     INITIAL_VOLUME,
     VOLUME_STEP,
@@ -217,6 +218,29 @@ async def test_remote_control_buttons(companion_client, companion_state, button)
     assert companion_state.latest_button == button
 
 
+async def test_remote_control_skip_forward_backward(companion_client, companion_state):
+    duration = companion_state.duration
+    await companion_client.remote_control.skip_forward()
+    await until(lambda: math.isclose(companion_state.duration, duration + 10))
+
+    duration = companion_state.duration
+    await companion_client.remote_control.skip_backward()
+    await until(lambda: math.isclose(companion_state.duration, duration - 10))
+
+    duration = companion_state.duration
+    await companion_client.remote_control.skip_forward(10.5)
+    await until(lambda: math.isclose(companion_state.duration, duration + 10.5))
+
+    duration = companion_state.duration
+    await companion_client.remote_control.skip_backward(7.3)
+    await until(lambda: math.isclose(companion_state.duration, duration - 7.3))
+
+    # "From beginning"
+    duration = companion_state.duration
+    await companion_client.remote_control.skip_backward(9999999.0)
+    await until(lambda: math.isclose(companion_state.duration, 0))
+
+
 async def test_audio_set_volume(companion_client, companion_state, companion_usecase):
     await until(lambda: companion_client.audio.volume, INITIAL_VOLUME)
 
diff --git a/tests/protocols/dmap/test_dmap_functional.py b/tests/protocols/dmap/test_dmap_functional.py
index 82130953..49d82f9c 100644
--- a/tests/protocols/dmap/test_dmap_functional.py
+++ b/tests/protocols/dmap/test_dmap_functional.py
@@ -270,6 +270,16 @@ class DMAPFunctionalTest(common_functional_tests.CommonFunctionalTests):
         await self.atv.remote_control.skip_backward()
         metadata = await self.playing()
         self.assertEqual(metadata.position, prev_position - SKIP_TIME)
+        prev_position = metadata.position
+
+        await self.atv.remote_control.skip_forward(13.0)
+        metadata = await self.playing()
+        self.assertEqual(metadata.position, prev_position + 13)
+        prev_position = metadata.position
+
+        await self.atv.remote_control.skip_backward(11.0)
+        metadata = await self.playing()
+        self.assertEqual(metadata.position, prev_position - 11)
 
     async def test_reset_revision_if_push_updates_fail(self):
         """Test that revision is reset when an error occurs during push update.
diff --git a/tests/protocols/mrp/test_mrp_functional.py b/tests/protocols/mrp/test_mrp_functional.py
index 0c2b3593..9c36cec6 100644
--- a/tests/protocols/mrp/test_mrp_functional.py
+++ b/tests/protocols/mrp/test_mrp_functional.py
@@ -474,6 +474,13 @@ class MRPFunctionalTest(common_functional_tests.CommonFunctionalTests):
         self.usecase.change_metadata(title="dummy4")
         metadata = await self.playing(title="dummy4")
         self.assertEqual(metadata.position, prev_position - 8)
+        prev_position = metadata.position
+
+        # Test specified skip time
+        await self.atv.remote_control.skip_forward(17)
+        self.usecase.change_metadata(title="dummy5")
+        metadata = await self.playing(title="dummy5")
+        self.assertEqual(metadata.position, prev_position + 17)
 
     async def test_button_play_pause(self):
         self.usecase.example_video(supported_commands=[CommandInfo_pb2.TogglePlayPause])