Mailing List Archive

Re: Ticket #12809: Use lastPlayPos rather than bookmark
On Wed, 27 Dec 2017 17:58:52 -0000
"MythTV" <noreply@mythtv.org> wrote:

> #12809: Use lastPlayPos rather than bookmark
> -------------------------------------+-----------------------------
> Reporter: rsiddons | Owner: rsiddons
> Type: Patch - Feature | Status: assigned
> Priority: minor | Milestone: 29.1
> Component: MythTV - Video Playback | Version: Master Head
> Severity: low | Resolution:
> Keywords: | Ticket locked: 0
> -------------------------------------+-----------------------------
>
> Comment (by pbennett):
>
> Roger
>
> Can I help with getting your patches in? I suggest applying them to
> master, rather than 29.1 as you had suggested.
>
> Regarding a service that can support the last played position. There
> are at the moment services !GetSavedBookmark and !SetSavedBookmark. I
> could add !GetLastPlayPos and !SetLastPlayPos for Kodi and others to
> use. It is time for me to learn about the MythTV services, so if you
> agree I can get going on that so that we can commit this stuff.
>
> Also there are some small changes in Piotr's version of the patches,
> which probably should also be included.
>
> Peter
>

Hi Peter,

Are you running out of things to fix?

Yes, it'll be good to get that cleared up.

I found an old patch (attached) that seems to apply cleanly.
It's never been tested though. I was hoping to avoid testing it
manually but my plans have been waylaid, as usual.
So you're most welcome to play with it.

It includes PROGSTART and others.
Seems to me a client would usually want several marks so it makes more
sense to group them together rather than cluttering the API and
requiring repeated calls. New marks can then be added easily.

I'm not aware of Piotr's mods.

I have seen occasional failures in the UI reported position but have
never bothered to debug it. Getting it committed would expedite that!

Regards
Re: Ticket #12809: Use lastPlayPos rather than bookmark [ In reply to ]
On 12/27/2017 06:05 PM, Roger Siddons wrote:
> On Wed, 27 Dec 2017 17:58:52 -0000
> "MythTV" <noreply@mythtv.org> wrote:
>
>> #12809: Use lastPlayPos rather than bookmark
>> -------------------------------------+-----------------------------
>> Reporter: rsiddons | Owner: rsiddons
>> Type: Patch - Feature | Status: assigned
>> Priority: minor | Milestone: 29.1
>> Component: MythTV - Video Playback | Version: Master Head
>> Severity: low | Resolution:
>> Keywords: | Ticket locked: 0
>> -------------------------------------+-----------------------------
>>
>> Comment (by pbennett):
>>
>> Roger
>>
>> Can I help with getting your patches in? I suggest applying them to
>> master, rather than 29.1 as you had suggested.
>>
>> Regarding a service that can support the last played position. There
>> are at the moment services !GetSavedBookmark and !SetSavedBookmark. I
>> could add !GetLastPlayPos and !SetLastPlayPos for Kodi and others to
>> use. It is time for me to learn about the MythTV services, so if you
>> agree I can get going on that so that we can commit this stuff.
>>
>> Also there are some small changes in Piotr's version of the patches,
>> which probably should also be included.
>>
>> Peter
>>
> Hi Peter,
>
> Are you running out of things to fix?
>
> Yes, it'll be good to get that cleared up.
>
> I found an old patch (attached) that seems to apply cleanly.
> It's never been tested though. I was hoping to avoid testing it
> manually but my plans have been waylaid, as usual.
> So you're most welcome to play with it.
>
> It includes PROGSTART and others.
> Seems to me a client would usually want several marks so it makes more
> sense to group them together rather than cluttering the API and
> requiring repeated calls. New marks can then be added easily.
>
> I'm not aware of Piotr's mods.
>
> I have seen occasional failures in the UI reported position but have
> never bothered to debug it. Getting it committed would expedite that!
>
> Regards
>
>
Piotr

I compared your first patch against Roger's and they were 99% the same.
You did make a few changes, I think you added something to a menu in one
place, plus some others. I would like you to separate out your changes
and give a small explanation of the purpose, so that when we are ready
we can commit Roger's code and then commit your changes after that. This
way we can easily identify where everything came from, and the reasons
for each change.

Peter

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Ticket #12809: Use lastPlayPos rather than bookmark [ In reply to ]
On 12/27/2017 07:35 PM, Peter Bennett wrote:
>> I'm not aware of Piotr's mods.
>>
>> I have seen occasional failures in the UI reported position but have
>> never bothered to debug it. Getting it committed would expedite that!
>>
>> Regards
>>
>>
> Piotr
>
> I compared your first patch against Roger's and they were 99% the
> same. You did make a few changes, I think you added something to a
> menu in one place, plus some others. I would like you to separate out
> your changes and give a small explanation of the purpose, so that when
> we are ready we can commit Roger's code and then commit your changes
> after that. This way we can easily identify where everything came
> from, and the reasons for each change.
>

Piotr

Roger's changes have many many conflicts when trying to apply them, due
to many changes in the system since they were created. I spent a couple
of hours fixing them, but still there were compile errors due to the new
setup. They were still using the old setup classes. I am going to take
your version. No need to try separating your changes out, it looks like
they are necessary to get past the conflicts.

Peter
Re: Ticket #12809: Use lastPlayPos rather than bookmark [ In reply to ]
>
> Piotr
>
> Roger's changes have many many conflicts when trying to apply them, due to many changes in the system since they were created. I spent a couple of hours fixing them, but still there were compile errors due to the new setup. They were still using the old setup classes. I am going to take your version. No need to try separating your changes out, it looks like they are necessary to get past the conflicts.
>
> Peter
>
Peter,
This is perfectly fine.
My work on #12809 was minimal needed just to get it working with current mythtv code.
It is so nice You are working on this.
milion thx!

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Ticket #12809: Use lastPlayPos rather than bookmark [ In reply to ]
On 12/27/2017 06:05 PM, Roger Siddons wrote:
> On Wed, 27 Dec 2017 17:58:52 -0000
> "MythTV"<noreply@mythtv.org> wrote:
>
>> #12809: Use lastPlayPos rather than bookmark
>> -------------------------------------+-----------------------------
>> Reporter: rsiddons | Owner: rsiddons
>> Type: Patch - Feature | Status: assigned
>> Priority: minor | Milestone: 29.1
>> Component: MythTV - Video Playback | Version: Master Head
>> Severity: low | Resolution:
>> Keywords: | Ticket locked: 0
>> -------------------------------------+-----------------------------
>>
>> Comment (by pbennett):
>>
>> Roger
>>
>> Can I help with getting your patches in? I suggest applying them to
>> master, rather than 29.1 as you had suggested.
>>
>> Regarding a service that can support the last played position. There
>> are at the moment services !GetSavedBookmark and !SetSavedBookmark. I
>> could add !GetLastPlayPos and !SetLastPlayPos for Kodi and others to
>> use. It is time for me to learn about the MythTV services, so if you
>> agree I can get going on that so that we can commit this stuff.
>>
>> Also there are some small changes in Piotr's version of the patches,
>> which probably should also be included.
>>
>> Peter
>>
> Hi Peter,
>
> Are you running out of things to fix?
>
> Yes, it'll be good to get that cleared up.
>
> I found an old patch (attached) that seems to apply cleanly.
> It's never been tested though. I was hoping to avoid testing it
> manually but my plans have been waylaid, as usual.
> So you're most welcome to play with it.
>
> It includes PROGSTART and others.
> Seems to me a client would usually want several marks so it makes more
> sense to group them together rather than cluttering the API and
> requiring repeated calls. New marks can then be added easily.
>
> I'm not aware of Piotr's mods.
>
> I have seen occasional failures in the UI reported position but have
> never bothered to debug it. Getting it committed would expedite that!
>
> Regards
>
>
>
Hi Roger

The old patch you provided for the services - I have it working but I
think it needs some more features. It only allows getting the marks, not
setting them. Kodi would also need to set the last played position so I
plan to add a set method for the 4 values: Bookmark, Duration,
Progstart, LastPlay. I don't know if you think allowing setting of
Duration or Progstart lets people shoot themselves in the foot. Is there
some consensus on what we should allow setting? Maybe it should only set
Bookmark and LastPlay?

Also I think the service should allow getting and setting using channel
and start time as well as by recordid. I will add those options.

Peter
Re: Ticket #12809: Use lastPlayPos rather than bookmark [ In reply to ]
On 01/10/2018 01:07 PM, Peter Bennett wrote:
> Hi Roger
>
> The old patch you provided for the services - I have it working but I
> think it needs some more features. It only allows getting the marks,
> not setting them. Kodi would also need to set the last played position
> so I plan to add a set method for the 4 values: Bookmark, Duration,
> Progstart, LastPlay. I don't know if you think allowing setting of
> Duration or Progstart lets people shoot themselves in the foot. Is
> there some consensus on what we should allow setting? Maybe it should
> only set Bookmark and LastPlay?
>
> Also I think the service should allow getting and setting using
> channel and start time as well as by recordid. I will add those options.
>
> Peter

I think it is easier to update the existing SetSavedBookmark service to
have an extra optional parameter BookmarkType that can take value
"lastplaypos" to update last play position. Since it is optional it will
continue to work without that parameter for existing applications that
update the bookmark.

Peter


_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Ticket #12809: Use lastPlayPos rather than bookmark [ In reply to ]
On Wed, 10 Jan 2018 13:07:40 -0500
Peter Bennett <pb.mythtv@gmail.com> wrote:

> On 12/27/2017 06:05 PM, Roger Siddons wrote:
> > On Wed, 27 Dec 2017 17:58:52 -0000
> > "MythTV"<noreply@mythtv.org> wrote:
> >
> >> #12809: Use lastPlayPos rather than bookmark
> >> -------------------------------------+-----------------------------
> >> Reporter: rsiddons | Owner: rsiddons
> >> Type: Patch - Feature | Status: assigned
> >> Priority: minor | Milestone: 29.1
> >> Component: MythTV - Video Playback | Version: Master Head
> >> Severity: low | Resolution:
> >> Keywords: | Ticket locked: 0
> >> -------------------------------------+-----------------------------
> >>
> Hi Roger
>
> The old patch you provided for the services - I have it working but I
> think it needs some more features. It only allows getting the marks,
> not setting them. Kodi would also need to set the last played
> position so I plan to add a set method for the 4 values: Bookmark,
> Duration, Progstart, LastPlay. I don't know if you think allowing
> setting of Duration or Progstart lets people shoot themselves in the
> foot. Is there some consensus on what we should allow setting? Maybe
> it should only set Bookmark and LastPlay?
>

Thanks for that Peter. SoapUI sounds useful.

I can't imagine a valid reason for a client to overwrite the duration,
so I think it should be excluded/ignored.

ProgStart is just a 'best guess' from the recorder, so I think a client
should be able to correct that (probably via user/cutlist editor).

> Also I think the service should allow getting and setting using
> channel and start time as well as by recordid. I will add those
> options.

I believe not. Many services were created when Chan/time was *the*
id of a recording and they had to be preserved when the *main*
id becrecordedId was added later. For new services it's not required -
recId is/should always be available.
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Ticket #12809: Use lastPlayPos rather than bookmark [ In reply to ]
On Wed, 10 Jan 2018 16:11:27 -0500
Peter Bennett <pb.mythtv@gmail.com> wrote:

> On 01/10/2018 01:07 PM, Peter Bennett wrote:
>
> I think it is easier to update the existing SetSavedBookmark service
> to have an extra optional parameter BookmarkType that can take value
> "lastplaypos" to update last play position. Since it is optional it
> will continue to work without that parameter for existing
> applications that update the bookmark.
>

Nice.

In another thread I've been wondering about allowing multiple bookmarks.
This patch doesn't look so clever now - it's predicated on a
single bookmark.

GetRecordedCommBreak & GetRecordedCutList both use a "Mark" int
already. I don't particularly like the 'int', but if we're going to use
it for the setter maybe GetMarks should simply return a CutList as well.
It would contain a list of all relevant marks except commflag/cutlist
ones. That would be more consistent & future-proof.

The client would have to search for a specific mark but that's not
onerous. And we can get rid of the new "Marks" type.

ints/enums in the API aren't ideal. We already use
"position"/"duration" so why not report the mark type as a string as
well ? There's a programtypes::toString already.
The strings would become part of the API spec but I think that would be
better than obscure ints. We need a way to document them though.

If you mail me your latest patch, I'll rework it.
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Ticket #12809: Use lastPlayPos rather than bookmark [ In reply to ]
On 11 January 2018 9:45:37 am Roger Siddons <dizygotheca@ntlworld.com> wrote:
......
>
> I can't imagine a valid reason for a client to overwrite the duration,
> so I think it should be excluded/ignored.

What about the pre-padding / post-padding being trimmed from the recording?


_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Ticket #12809: Use lastPlayPos rather than bookmark [ In reply to ]
On 11/01/18 05:26, Mark Perkins wrote:
> On 11 January 2018 9:45:37 am Roger Siddons <dizygotheca@ntlworld.com> wrote:
> ......
>>
>> I can't imagine a valid reason for a client to overwrite the duration,
>> so I think it should be excluded/ignored.
>
> What about the pre-padding / post-padding being trimmed from the recording?
>

Yes. I don't know how it fits into the new plans, but duration can/will
be reset by mythcommflag --rebuild

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Ticket #12809: Use lastPlayPos rather than bookmark [ In reply to ]
On Thu, 11 Jan 2018 09:47:20 +0000
John Pilkington <J.Pilk@tesco.net> wrote:

> On 11/01/18 05:26, Mark Perkins wrote:
> > On 11 January 2018 9:45:37 am Roger Siddons
> > <dizygotheca@ntlworld.com> wrote: ......
> >>
> >> I can't imagine a valid reason for a client to overwrite the
> >> duration, so I think it should be excluded/ignored.
> >
> > What about the pre-padding / post-padding being trimmed from the
> > recording?
>
> Yes. I don't know how it fits into the new plans, but duration
> can/will be reset by mythcommflag --rebuild
>

Good question.

Any form of 'transcode' invalidates all marks so a traditional
frontend should probably never set the duration directly, but prod the
backend to do it, via mythutils/mythcommflag.

If the API supports mythutil-like clients as well - we let
any client do anything - then there's a risk of corrupted
recordings (albeit repairable)

I would argue that bookmark/lastplaypos etc are user/client properties
that only affect playback behaviour. They don't cause damage.

Whereas duration/seek-table/etc marks are backend properties and only
updateable via approved methods, i.e. mythutils/mythcommflag.
That they are all "marks" is an implementation detail.

A 'set duration to 0' request could even become a sentinel to run
mythutils/mythcommflag, but I'm not sure that's a good idea.

Maybe a new Myth service ? I can't find any current service for a client
to run a User Job or start commflagging.
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org