[vlc-devel] [PATCH] video_output: rename next_system_pts

Alexandre Janniaux ajanni at videolabs.io
Thu Feb 4 10:19:40 UTC 2021


On Thu, Feb 04, 2021 at 10:56:36AM +0100, Steve Lhomme wrote:
> On 2021-02-04 10:47, Alexandre Janniaux wrote:
> > Hi,
> >
> > On Thu, Feb 04, 2021 at 10:31:34AM +0100, Steve Lhomme wrote:
> > > On 2021-02-04 10:23, Alexandre Janniaux wrote:
> > > > Hi,
> > > >
> > > > I'm not sure system_current_pts is less confusing than
> > > > next_system_pts. Maybe a variation including the words
> > > > «system» to indicate the reference, and «deadline» to
> > > > indicate that the timestamp must be reached is better?
> > > >
> > > > Maybe something like system_render_deadline ?
> > >
> > > There's no indication this system vlc_tick_t is for the current of the next
> > > picture, which is what this patch is trying to fix.
> > >
> > > I also dislike the word deadline in general. I only kept it for
> > > compatibility. It's not a time that the rendering should happen any time
> > > before that. It's actually the exact time the display swap should occur.
> > >
> > > How about "system_current_swap_pts" ?
> >
> > Actually, it might be easier to find an alternative if
> > I indicate why I don't think it's less confusing.
> >
> > Basically you wrote:
> >
> > > It is a confusing name as it's now the system PTS of displayed.current.
> >
> > But this date is still likely in the future, so I find that
> > using «current» here is more confusing than using «next».
>
> Not with "system_current_swap_pts", it is the swap time that is currently
> set to be used. It's also the system PTS of the current picture.

Documentation-wise, it makes sense, but if you mention confusion,
it's still confusing with "the time when the swap has happened"
because of "current".

I agree that deadline has some unwanted semantic, but I don't
feel like what you suggest is clearer than the current code.

Regards,
--
Alexandre Janniaux
Videolabs

> > > > Regards,
> > > > --
> > > > Alexandre Janniaux
> > > > Videolabs
> > > >
> > > > On Thu, Feb 04, 2021 at 09:46:50AM +0100, Steve Lhomme wrote:
> > > > > It is a confusing name as it's now the system PTS of displayed.current.
> > > > >
> > > > > Also rework the late test to be more readable.
> > > > > ---
> > > > >    src/video_output/video_output.c | 7 +++----
> > > > >    1 file changed, 3 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
> > > > > index 323b40c0846..cc24a74a5a7 100644
> > > > > --- a/src/video_output/video_output.c
> > > > > +++ b/src/video_output/video_output.c
> > > > > @@ -1521,13 +1521,12 @@ static int ThreadDisplayPicture(vout_thread_sys_t *vout, vlc_tick_t *deadline)
> > > > >            }
> > > > >            else if (!paused)
> > > > >            {
> > > > > -            const vlc_tick_t next_system_pts =
> > > > > +            const vlc_tick_t system_current_pts =
> > > > >                    vlc_clock_ConvertToSystem(sys->clock, system_now,
> > > > >                                              sys->displayed.current->date, sys->rate);
> > > > > -            if (likely(next_system_pts != INT64_MAX))
> > > > > +            if (likely(system_current_pts != INT64_MAX))
> > > > >                {
> > > > > -                vlc_tick_t date_next = next_system_pts - render_delay;
> > > > > -                if (date_next <= system_now)
> > > > > +                if (system_now + render_delay >= system_current_pts)
> > > > >                    {
> > > > >                        // the current frame will be late, look for the next not late one
> > > > >                        next =
> > > > > --
> > > > > 2.29.2
> > > > >
> > > > > _______________________________________________
> > > > > vlc-devel mailing list
> > > > > To unsubscribe or modify your subscription options:
> > > > > https://mailman.videolan.org/listinfo/vlc-devel
> > > > _______________________________________________
> > > > vlc-devel mailing list
> > > > To unsubscribe or modify your subscription options:
> > > > https://mailman.videolan.org/listinfo/vlc-devel
> > > >
> > > _______________________________________________
> > > vlc-devel mailing list
> > > To unsubscribe or modify your subscription options:
> > > https://mailman.videolan.org/listinfo/vlc-devel
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> >
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel


More information about the vlc-devel mailing list