[builtin] adds new builtin for seeking #6801

xhaggi · 2015-03-23T16:31:49Z

This adds a new built-in command Seek(<seconds>) to support instant seeking in the current playing media file. @da-anda it should make every small step lover happy ;)

<keymap>
  <FullscreenVideo>
    <keyboard>
      <YOURKEY>Seek(-7)</YOURKEY>
    </keyboard>
  <FullscreenVideo>
</keymap>

Related discussion at http://forum.kodi.tv/showthread.php?tid=217616&pid=1957016#pid1957016

xbmc/utils/SeekHandler.cpp

+  m_percent = m_percentPlayTime + percentPerSecond * seconds;
+  g_infoManager.SetSeekStepSize(seconds);
+
+  if (m_percent > 100.0f) m_percent = 100.0f;


da-anda · 2015-03-23T16:44:15Z

nice, thanks. Should we extend JSON-Rpc to also support it? It already provides seeking by percentage, absolute time code and small/bigsteps, but no relative seeking with a custom step size. From a quick look it might be a bit tricky to add it there (requires more advanced evaluation of the input params)

Tolriq · 2015-03-23T17:38:16Z

JSON should be able to call the Seek with Input.Action so not a problem for remote makers I guess to use this.

xbmc/utils/SeekHandler.cpp

+  if (g_infoManager.GetTotalPlayTime())
+    percentPerSecond = 100.0f / (float)g_infoManager.GetTotalPlayTime();
+
+  m_percent = m_percentPlayTime + percentPerSecond * seconds;


Paxxi · 2015-03-23T18:13:41Z

What exactly is this trying to solve? looking at the forum thread I get the impression it's to satisfy one users wish to retain old behaviour?

Furthermore, with this being callable from Builtins raises the question of thread safety, there's no protections around state modifications and I'm not sure we know which thread we're called on anymore.

xbmc/utils/SeekHandler.cpp

+  m_analogSeek = false;
+  m_seekDelay = 0;
+
+  if (g_infoManager.GetTotalPlayTime())


Paxxi · 2015-03-23T18:17:57Z

also please use c++ casts and not old style c casts, see discussion #6791

Montellese · 2015-03-23T19:52:05Z

@Tolriq: JSON-RPC doesn't allow calling the builtins directly and obviously it would be better to intergrate this into Player.Seek if it is added.

xhaggi · 2015-03-23T19:53:29Z

@Paxxi hope i'll have addressed all your comments. I've re-worked the whole method + make it thread-safe.
@FernetMenta added a cosmetic commit which breaks the statements to new line in Seek()

Paxxi · 2015-03-23T20:23:02Z

On ipad but looks good from what i see @xhaggi

xhaggi · 2015-03-23T20:26:42Z

@Montellese if we want to add this to Player.Seek (JSON-RPC) what do you think about if we differ between seconds and percent by adding a %-sign to the percent value?

Tolriq · 2015-03-24T08:54:01Z

@xhaggi this completely breaks the API without any backward compatibility.

xhaggi · 2015-03-24T09:01:56Z

@Tolriq better we leave everything as it is and use "10s" or "-7s" for seek by seconds?

Tolriq · 2015-03-24T09:09:13Z

Sounds better for compatibility and easy to use for remote makers.

But not sure about documentation for new comers, this make the value field quite complex in all it's states now.

@Montellese call but why not an string value like "relative" and a new parameter like "amount" ? Easy to follow and document and avoid mixing things when input is a string.

xhaggi · 2015-03-24T09:13:10Z

if we introduce new parameters we should use a separate one for each condition:

percent
seconds
step
time

This way we have backward compatibility and a clear set of parameters (self-explained)

Tolriq · 2015-03-24T09:32:37Z

Yes that's the idea :) Just need to keep compatibility too.

da-anda · 2015-03-24T10:01:44Z

@xhaggi AFAIK you have to add the new JSON-RPC parameter to the schema description as well: https://github.com/xbmc/xbmc/blob/master/xbmc/interfaces/json-rpc/schema/methods.json#L284

and define a new type ofc: https://github.com/xbmc/xbmc/blob/master/xbmc/interfaces/json-rpc/schema/types.json#L175

xhaggi · 2015-03-24T10:09:08Z

@da-anda thanks for the hint, never touched that section ;). If fine by @Montellese I'll add this.

NedScott · 2015-03-25T00:53:10Z

@Paxxi Actually, this feature has long since been requested on the forums, long before the current v15 seeking changes. This is one stone that kills many birds.

Montellese · 2015-03-25T07:56:37Z

Checkout Montellese@4990799 as a replacement for your last commit. It adds explicitly named properties to the value parameter of Player.Seek like you mentioned above but also keeps the currently supported values. That way percentage, time and step can be used either as a pure value or as a named property and seconds is only supported as a named property.

This is completely runtime untested (only compile tested).

xhaggi · 2015-03-25T07:59:30Z

@Montellese great, i'll cherry-pick but couldn't test it by myself. @Tolriq if we provide a test-build could you do some tests?

Tolriq · 2015-03-25T08:44:02Z

I can easily test the no regressions and do quick test about new functions yes, and I promise I'll try to find time to build a VM to be able to build again :)

Montellese · 2015-03-25T08:46:32Z

@Tolriq: I triggered a test build at http://jenkins.kodi.tv/job/WIN-32/4204/.

xhaggi · 2015-03-25T13:00:47Z

Test-Build:
http://mirrors.xbmc.org/test-builds/win32/KodiSetup-20150325-3f8a5c1-builtin-seek-support.exe

Tolriq · 2015-04-01T08:06:23Z

@xhaggi I had tested regression yesterday and there's things broken (like move to specific time) but had no time to make an investigation on what the problem is :(

Anyway here's the logs from Yatse :

As you can see hours / minutes are lost, seeking by percentage still work, and no time to test the new parameters.

JsonRpc.doRequest@243: Request : {"jsonrpc":"2.0","id":1,"method":"Player.Seek","params":{"playerid":1,"value":{"hours":1,"minutes":30,"seconds":49}}}
JSON TCP message : {"jsonrpc":"2.0","method":"Player.OnSeek","params":{"data":{"item":{"id":7,"type":"movie"},"player":{"playerid":1,"seekoffset":{"hours":0,"milliseconds":0,"minutes":0,"seconds":49},"speed":1,"time":{"hours":0,"milliseconds":103,"minutes":31,"seconds":38}}},"sender":"xbmc"}}

It seems I had an attak on my webserver so no time to do more until I fix it :(

xhaggi · 2015-04-01T08:12:42Z

@Tolriq thanks anyway 👍 will take a look at it

xhaggi · 2015-04-01T08:31:51Z

@Montellese after a quick look at your changes I don't think this is a newly introduced issue. Would be nice if you could take a look too.

xhaggi · 2015-04-03T08:34:04Z

@Montellese ping

Montellese · 2015-04-03T09:12:49Z

Sorry I won't be able to look into it until Monday earliest.

xhaggi · 2015-04-07T10:43:49Z

okay, let me know what's wrong so i can fix it ;)

xhaggi · 2015-04-08T13:54:42Z

@Montellese don't forget to take a look ;)

xhaggi · 2015-04-09T14:15:50Z

@Montellese if you don't find time I'll drop your commit so this could be merged for BETA1.

Montellese · 2015-04-09T14:24:52Z

@xhaggi: I should find time tomorrow evening. But feel free to drop my commit if you want to merge it ASAP.

xhaggi · 2015-04-09T14:28:47Z

i don't want to wait to long because i'm not sure when we close the merge window ;)

Montellese · 2015-04-09T21:12:18Z

@xhaggi: Sorry for the delay. Please pick and squash Montellese@0d18c4d. There was a small mistake that broke time based seeking (and some bad formatting).

…eeking by a number of seconds

xhaggi · 2015-04-10T06:17:18Z

@Montellese thank you for the fix ;)

jenkins build this please

xhaggi · 2015-04-10T06:19:11Z

@Tolriq it would be really nice if you could test it again, if not no problem we'll merge it in and fix it later.

xhaggi · 2015-04-10T17:13:43Z

I've added a new commit which improves the "instant" seek support (if you disabled the seek delay).
As the seek is instantly processed we need to drop each call to Seek() during this process.

jenkins build this please

da-anda · 2015-04-10T17:22:28Z

shouldn't the seek fix be a separate PR? ;)

xhaggi · 2015-04-10T17:36:03Z

mhhhh it does not really fix something it's more an improvement and I'm lazy ;)

xhaggi · 2015-04-10T17:39:31Z

@da-anda BTW instant seek is used by the seek built-in so it's related to this PR ;)

xhaggi · 2015-04-11T07:38:48Z

jenkins build and merge

FernetMenta reviewed Mar 23, 2015
View reviewed changes

xbmc/utils/SeekHandler.cpp

m_percent = m_percentPlayTime + percentPerSecond * seconds;

g_infoManager.SetSeekStepSize(seconds);

if (m_percent > 100.0f) m_percent = 100.0f;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

xhaggi added the Type: Improvement non-breaking change which improves existing functionality label Mar 23, 2015

Paxxi reviewed Mar 23, 2015
View reviewed changes

xbmc/utils/SeekHandler.cpp

m_analogSeek = false;

m_seekDelay = 0;

if (g_infoManager.GetTotalPlayTime())

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

xhaggi force-pushed the builtin-seek-support branch from 06d17e9 to d1346d3 Compare March 23, 2015 19:44

xhaggi force-pushed the builtin-seek-support branch 2 times, most recently from e100276 to 309aa6f Compare March 23, 2015 20:18

xhaggi force-pushed the builtin-seek-support branch from 5c2f641 to cb27acc Compare March 24, 2015 09:07

xhaggi force-pushed the builtin-seek-support branch from cb27acc to 3f8a5c1 Compare March 25, 2015 08:03

xhaggi added 3 commits April 8, 2015 15:55

[builtin] adds new builtin for seeking

e79e208

[cpp] use c++ style casts in CSeekHandler

abb5f03

[cosmetic] put statement of single line if to new line

8e29005

xhaggi force-pushed the builtin-seek-support branch from 0ce3e89 to 1a25691 Compare April 8, 2015 13:57

jsonrpc: add explicitly named parameters to Player.Seek and support s…

3fd4e1a

…eeking by a number of seconds

xhaggi force-pushed the builtin-seek-support branch from 1a25691 to 3fd4e1a Compare April 10, 2015 06:16

[seek] improves instant seek in seek handler

a69dde3

xhaggi force-pushed the builtin-seek-support branch from 64c51c2 to a69dde3 Compare April 11, 2015 07:38

jenkins4kodi added a commit that referenced this pull request Apr 11, 2015

Merge pull request #6801 from xhaggi/builtin-seek-support

7c04112

jenkins4kodi merged commit 7c04112 into xbmc:master Apr 11, 2015

xhaggi deleted the builtin-seek-support branch April 20, 2015 10:52

Tolriq mentioned this pull request Apr 12, 2020

[jsonrpc] [breakingchange]remove ambiguous and duplicate Player.Seek options #15939

Merged

2 tasks

[builtin] adds new builtin for seeking #6801

[builtin] adds new builtin for seeking #6801

Conversation

xhaggi commented Mar 23, 2015

This comment was marked as spam.

This comment was marked as spam.

da-anda commented Mar 23, 2015

Tolriq commented Mar 23, 2015

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Paxxi commented Mar 23, 2015

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Paxxi commented Mar 23, 2015

Montellese commented Mar 23, 2015

xhaggi commented Mar 23, 2015

Paxxi commented Mar 23, 2015

xhaggi commented Mar 23, 2015

Tolriq commented Mar 24, 2015

xhaggi commented Mar 24, 2015

Tolriq commented Mar 24, 2015

xhaggi commented Mar 24, 2015

Tolriq commented Mar 24, 2015

da-anda commented Mar 24, 2015

xhaggi commented Mar 24, 2015

NedScott commented Mar 25, 2015

Montellese commented Mar 25, 2015

xhaggi commented Mar 25, 2015

Tolriq commented Mar 25, 2015

Montellese commented Mar 25, 2015

xhaggi commented Mar 25, 2015

Tolriq commented Apr 1, 2015

xhaggi commented Apr 1, 2015

xhaggi commented Apr 1, 2015

xhaggi commented Apr 3, 2015

Montellese commented Apr 3, 2015

xhaggi commented Apr 7, 2015

xhaggi commented Apr 8, 2015

xhaggi commented Apr 9, 2015

Montellese commented Apr 9, 2015

xhaggi commented Apr 9, 2015

Montellese commented Apr 9, 2015

xhaggi commented Apr 10, 2015

xhaggi commented Apr 10, 2015

xhaggi commented Apr 10, 2015

da-anda commented Apr 10, 2015

xhaggi commented Apr 10, 2015

xhaggi commented Apr 10, 2015

xhaggi commented Apr 11, 2015