dvdplayer/pvr: fix seek/ff/rw for PVR, calculate offset to pts on screen #2107

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@FernetMenta
Member

FernetMenta commented Jan 23, 2013

With approximately 8 seconds in the dvd queues a small step back of 5 secs resulted in a step forward of 3 secs.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 23, 2013

Member

Have you tested this for bluray and dvds?

Member

elupus commented Jan 23, 2013

Have you tested this for bluray and dvds?

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 23, 2013

Member

Also conceptually the change is wrong (time offset is a way to relate time
from a packet from input between dts and display time) thus normally it us
fully constant during playback.

With this change it is now dependent on queue size.

I would rather see a proper fix for display time. It should travers the
queue together with packets.

Member

elupus commented Jan 23, 2013

Also conceptually the change is wrong (time offset is a way to relate time
from a packet from input between dts and display time) thus normally it us
fully constant during playback.

With this change it is now dependent on queue size.

I would rather see a proper fix for display time. It should travers the
queue together with packets.

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Jan 23, 2013

Member

Sorry, I don't follow. How is display time related. For seeking we need dts. For demuxers time is set to GetClock which already reflects time on screen.

IMO this is already over-complicated. There's been two fixes for this within the past 2 weeks and its still broken. Why bothering with offsets when all required is simply taking the time on screen. Just like ffmpeg input stream does it.

Member

FernetMenta commented Jan 23, 2013

Sorry, I don't follow. How is display time related. For seeking we need dts. For demuxers time is set to GetClock which already reflects time on screen.

IMO this is already over-complicated. There's been two fixes for this within the past 2 weeks and its still broken. Why bothering with offsets when all required is simply taking the time on screen. Just like ffmpeg input stream does it.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 23, 2013

Member

The dts is not continuous on dvds and blurays and don't relate at all to
the current time in the played title. Thus we need to relate the two.

Member

elupus commented Jan 23, 2013

The dts is not continuous on dvds and blurays and don't relate at all to
the current time in the played title. Thus we need to relate the two.

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Jan 23, 2013

Member

Ok, so for absolute seeking by clicking on the progress bar display time is needed, right? The dilemma for pvr streams is that display time is actual time and not accurately related to dts.

Member

FernetMenta commented Jan 23, 2013

Ok, so for absolute seeking by clicking on the progress bar display time is needed, right? The dilemma for pvr streams is that display time is actual time and not accurately related to dts.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 23, 2013

Member

Yup. That is the same for dvd. So we do have the same dilemma. But
currently the big issue is that GetTime in IPlayer return display time as
sampled from demuxer/input not what is currently on screen (taking queues
into accouny).

So the real issue is that what is displayed as current time is not really
what is currently displayed.

If you fix that, seeking aught to work with current code.

Member

elupus commented Jan 23, 2013

Yup. That is the same for dvd. So we do have the same dilemma. But
currently the big issue is that GetTime in IPlayer return display time as
sampled from demuxer/input not what is currently on screen (taking queues
into accouny).

So the real issue is that what is displayed as current time is not really
what is currently displayed.

If you fix that, seeking aught to work with current code.

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Jan 23, 2013

Member

now it corrects display time by the timespan in the queue if clock source is not ETIMESOURCE_CLOCK

UpdateCorrection takes care that we don't have gaps in the queue right? So we don't need to consider this?

Member

FernetMenta commented Jan 23, 2013

now it corrects display time by the timespan in the queue if clock source is not ETIMESOURCE_CLOCK

UpdateCorrection takes care that we don't have gaps in the queue right? So we don't need to consider this?

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 23, 2013

Member

Normally it is without gaps now a day. You could have used the demuxer queues GetTimeSize() function to get how much data is in them. Should be more reliable.

However, I would much rather see that we tag demuxer packets with display time, so we can actually always get the current display time in sync with what players are displaying.

Member

elupus commented Jan 23, 2013

Normally it is without gaps now a day. You could have used the demuxer queues GetTimeSize() function to get how much data is in them. Should be more reliable.

However, I would much rather see that we tag demuxer packets with display time, so we can actually always get the current display time in sync with what players are displaying.

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Jan 23, 2013

Member

so we can actually always get the current display time in sync with what players are displaying.

hmm, video player actually does not display anything. To make this strategy work properly we would need to tag DVDVideoPicture and render buffer.

dvdMessageQueue.GetTimeSize is not accurate either, why not asking RenderManager? It knows best what timestamp we have on screen.

Member

FernetMenta commented Jan 23, 2013

so we can actually always get the current display time in sync with what players are displaying.

hmm, video player actually does not display anything. To make this strategy work properly we would need to tag DVDVideoPicture and render buffer.

dvdMessageQueue.GetTimeSize is not accurate either, why not asking RenderManager? It knows best what timestamp we have on screen.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 23, 2013

Member

We don't want dts time stamp... We want display time. Display time is
currently not available at any other stage than right after demuxing. So
first it need to go along up to avplayers. Then maybe (a big maybe) send
that along to render managers, but I see no point for that.

Member

elupus commented Jan 23, 2013

We don't want dts time stamp... We want display time. Display time is
currently not available at any other stage than right after demuxing. So
first it need to go along up to avplayers. Then maybe (a big maybe) send
that along to render managers, but I see no point for that.

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Jan 23, 2013

Member

So the strategy is tagging the packets with display time and read those values back in UpdatePlayState:

state.time = m_dvdPlayerXXX.GetDisplayTime()

Do I get this right?

Member

FernetMenta commented Jan 23, 2013

So the strategy is tagging the packets with display time and read those values back in UpdatePlayState:

state.time = m_dvdPlayerXXX.GetDisplayTime()

Do I get this right?

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 23, 2013

Member

Yeah something like that.

Member

elupus commented Jan 23, 2013

Yeah something like that.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 23, 2013

Member

But I think we need some more stuff as well like chapter. So best is you
push a separate message down containing a struct of data. When the
avplayers get it, they timestamp it and put it pack up to dvdplayer in its
message queue.

Member

elupus commented Jan 23, 2013

But I think we need some more stuff as well like chapter. So best is you
push a separate message down containing a struct of data. When the
avplayers get it, they timestamp it and put it pack up to dvdplayer in its
message queue.

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Jan 24, 2013

Member

I have updated the pr. Could you have a look?

Member

FernetMenta commented Jan 24, 2013

I have updated the pr. Could you have a look?

@davilla

This comment has been minimized.

Show comment Hide comment
@davilla

davilla Jan 24, 2013

Contributor

just curious, why not use CDVDClock, I thought it represented the current display time.

Contributor

davilla commented Jan 24, 2013

just curious, why not use CDVDClock, I thought it represented the current display time.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 24, 2013

Member

It represent current PTS from demuxer. But for dvd's this is not the same
as progress through title, nor is it for live recordings. (this is the
whole issue)

On Thu, Jan 24, 2013 at 6:01 PM, davilla notifications@github.com wrote:

just curious, why not use CDVDClock, I thought it represented the current
display time.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/2107#issuecomment-12661187.

Member

elupus commented Jan 24, 2013

It represent current PTS from demuxer. But for dvd's this is not the same
as progress through title, nor is it for live recordings. (this is the
whole issue)

On Thu, Jan 24, 2013 at 6:01 PM, davilla notifications@github.com wrote:

just curious, why not use CDVDClock, I thought it represented the current
display time.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/2107#issuecomment-12661187.

@davilla

This comment has been minimized.

Show comment Hide comment
@davilla

davilla Jan 24, 2013

Contributor

humm, maybe we should change it's name to CDVDDemuxerClock then :)

Contributor

davilla commented Jan 24, 2013

humm, maybe we should change it's name to CDVDDemuxerClock then :)

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Jan 26, 2013

Member

@elupus This works for me on demuxer streams, dvd, and bluray (at least the scenarios I have tested)

I need this working, otherwise it's useless going ahead with time shift on PVR.

b979d144950db3714ac387ec1397eb85f0b402ba fixes another long known issue that rewind did go wrong speed. Syncing players on catch-up seeks sets the clock and causes rewind going much too fast.

Member

FernetMenta commented Jan 26, 2013

@elupus This works for me on demuxer streams, dvd, and bluray (at least the scenarios I have tested)

I need this working, otherwise it's useless going ahead with time shift on PVR.

b979d144950db3714ac387ec1397eb85f0b402ba fixes another long known issue that rewind did go wrong speed. Syncing players on catch-up seeks sets the clock and causes rewind going much too fast.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 26, 2013

Member

Please hold of on that rewind issue until we have the display time handling in place. I understand the issue, but i need to think about how it affects other things and if we can avoid complicating the code further.

Member

elupus commented Jan 26, 2013

Please hold of on that rewind issue until we have the display time handling in place. I understand the issue, but i need to think about how it affects other things and if we can avoid complicating the code further.

@elupus

View changes

xbmc/cores/dvdplayer/DVDMessage.h
+private:
+ DisplayTime *m_displayTime;
+};
+

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 26, 2013

Member

The template version should work perfectly fine for an assignable structure

@elupus

elupus Jan 26, 2013

Member

The template version should work perfectly fine for an assignable structure

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Jan 26, 2013

Member

@elupus I have updated the pr according to your comments. I add the time it takes for sending back the structure to display time. Is that what you meant? What can block dvdplayer message processing that this matters?

Member

FernetMenta commented Jan 26, 2013

@elupus I have updated the pr according to your comments. I add the time it takes for sending back the structure to display time. Is that what you meant? What can block dvdplayer message processing that this matters?

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 27, 2013

Member

Input stream reading can block dvdplayer thread for quite some time (slow/stalled network and similar..). So it can take a while before it gets the returned packet.

Looking much better. I'm trying to decide if i like/need the split between where the returned packet data is stored.

Member

elupus commented Jan 27, 2013

Input stream reading can block dvdplayer thread for quite some time (slow/stalled network and similar..). So it can take a while before it gets the returned packet.

Looking much better. I'm trying to decide if i like/need the split between where the returned packet data is stored.

+ {
+ m_CurrentVideo.displayTime = dTime;
+ }
+ }

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 27, 2013

Member

Please update the state directly here when you receive the display time callback.

@elupus

elupus Jan 27, 2013

Member

Please update the state directly here when you receive the display time callback.

@@ -75,6 +76,8 @@ class CCurrentStream
// stuff to handle starting after seek
double startpts;
+ DisplayTime displayTime;
+

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 27, 2013

Member

This is not needed if you just update state when we receive the callback.

@elupus

elupus Jan 27, 2013

Member

This is not needed if you just update state when we receive the callback.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 27, 2013

Member

I'm doing some tests here. I have an idea of sending down the whole SPlayerState structure and echoing it back again. Should be simpler i think. Will report back how it fares.

Member

elupus commented Jan 27, 2013

I'm doing some tests here. I have an idea of sending down the whole SPlayerState structure and echoing it back again. Should be simpler i think. Will report back how it fares.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 27, 2013

Member

I think i like the following better: http://paste.debian.net/229337/

Will get us the full demuxer state available at the time of display. Give it a go. (if it works alright for PVR we can start looking at the other issue)

Member

elupus commented Jan 27, 2013

I think i like the following better: http://paste.debian.net/229337/

Will get us the full demuxer state available at the time of display. Give it a go. (if it works alright for PVR we can start looking at the other issue)

@FernetMenta

This comment has been minimized.

Show comment Hide comment
@FernetMenta

FernetMenta Jan 27, 2013

Member

Great, will test it tomorrow.
This does also consider m_offset_pts, right? Just encountered a problem while testing without considering it.

Member

FernetMenta commented Jan 27, 2013

Great, will test it tomorrow.
This does also consider m_offset_pts, right? Just encountered a problem while testing without considering it.

@elupus

This comment has been minimized.

Show comment Hide comment
@elupus

elupus Jan 27, 2013

Member

Yea it should. I spotted some bugs with that while at it: #2127

Member

elupus commented Jan 27, 2013

Yea it should. I spotted some bugs with that while at it: #2127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment