[vlc-devel] [PATCH v2] Reset p_context->skip_frame when done seeking

[Marc Aldorasi]

On Mon, Aug 19, 2019 at 8:39 AM Steve Lhomme <robux4 at ycbcr.xyz> wrote:
>
> Hi,
>
> On 2019-08-06 5:39, Marc Aldorasi wrote:
> > Previously, if p_sys->b_hurry_up was false and we weren't seeking, the
> > value of p_context->skip_frame would not be modified.  This means that
> > if we started seeking we would set p_context->skip_frame to
> > AVDISCARD_NONREF, and then when we finished seeking we wouldn't change
> > p_context->skip_frame and it would still be skipping non-reference
> > frames, causing choppy playback.  This patch resets
> > p_context->skip_frame when we're not seeking so playback is smooth.
>
> The p_context->skip_frame should indeed be reset once seeking is done
> and that's currently not the case if b_hurry_up is false. But it should
> reset it to AVDISCARD_DEFAULT which is the value that is used when
> p_sys->b_hurry_up is false, not the p_sys->i_skip_frame value. Otherwise
> that makes the "avcodec-hurry-up" option useless since it would always
> be applied.

I'm a bit confused by this.  In response to the last patch you wrote

> It's a good idea to reset the skip_frame once seeking is done, but it
> should revert to "p_sys->i_skip_frame" so that it respects the
> "avcodec-skip-frame" variable.

which I took to mean that it should always be respecting
avcodec-skip-frame.  If I follow the latest suggestion, I'm concerned it
contradicts the previous suggestion.

> By default the "avcodec-hurry-up" option is true. That means people who
> set it to false really don't want to skip more than non-ref frames.
>
> On the other hand to achieve that they could also use
> --avcodec-skip-frame=1 and it would have the same effect.
>
> So if you want to go that way you should also remove (make obsolete) the
> avcodec-hurry-up option.

The avcodec-hurry-up option still guards the call to
filter_earlydropped_blocks, so I'd prefer not to go that way.

> By the way, the second part of this test in the current code is useless,
> FRAMEDRO_NONREF is never used/set in the code:
>
>    if( !b_need_output_picture || p_sys->framedrop == FRAMEDROP_NONREF )

Good catch, I'll remove that.

> > ---
> >   modules/codec/avcodec/video.c | 14 ++++++--------
> >   1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/modules/codec/avcodec/video.c b/modules/codec/avcodec/video.c
> > index 2232b7a291..d9be9d4a61 100644
> > --- a/modules/codec/avcodec/video.c
> > +++ b/modules/codec/avcodec/video.c
> > @@ -1031,15 +1031,13 @@ static int DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> >       else
> >           b_need_output_picture = false;
> >
> > -    /* Change skip_frame config only if hurry_up is enabled */
> > -    if( p_sys->b_hurry_up )
> > -    {
> > -        p_context->skip_frame = p_sys->i_skip_frame;
> > +    p_context->skip_frame = p_sys->i_skip_frame;
> >
> > -        /* Check also if we should/can drop the block and move to next block
> > -            as trying to catchup the speed*/
> > -        if( p_dec->b_frame_drop_allowed )
> > -            p_block = filter_earlydropped_blocks( p_dec, p_block );
> > +    /* Check if we should/can drop the block and move to the next block
> > +       to try to catchup */
> > +    if( p_sys->b_hurry_up && p_dec->b_frame_drop_allowed )
> > +    {
> > +        p_block = filter_earlydropped_blocks( p_dec, p_block );
> >       }
> >
> >       if( !b_need_output_picture || p_sys->framedrop == FRAMEDROP_NONREF )
> > --
> > 2.22.0
> >