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

[Steve Lhomme]

On 2019-08-20 14:46, Marc Aldorasi wrote:
> On Tue, Aug 20, 2019 at 12:59 AM Steve Lhomme <robux4 at ycbcr.xyz> wrote:
>>
>>
>>
>> On 2019-08-20 3:26, Marc Aldorasi wrote:
>>> 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.
> 
> While writing this patch, I discovered that this is not the case.
> p_context->skip_frame is initialized to the value of
> avcodec-skip-frame without regard to the value of avcodec-hurry-up
> (see video.c:583).  So there are 2 options:
> 
> 1. Respect the value of avcodec-skip-frame unconditionally (the
> current behavior when not seeking), making avcodec-hurry-up only
> control filtering early blocks.
> 2. Respect the value of avcodec-skip-frame only when avcodec-hurry-up
> is set, causing it to enable both frame skipping and filtering early
> blocks, and use AVDISCARD_DEFAULT when avcodec-hurry-up is not set.
> 
> Which of these is the preferred behavior?  I'm leaning towards 1, but
> both make sense to me.

I'd go for option 1 as well.

>>>
>>> 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.
>>
>> Ah yes, I missed that part. So never mind, we still keep both options.
>> We just need to reset properly p_context->skip_frame in both modes then.
>>
>>>> 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
>>>>>
>>> _______________________________________________
>>> 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
>