[vlc-devel] [PATCH 08/10] vout: don't clobber crop borders when updating

[Steve Lhomme]

On 2021-02-06 17:27, remi at remlab.net wrote:
> From: Rémi Denis-Courmont <remi at remlab.net>
> 
> The vout_UpdateSourceCrop() function uses the crop borders as both
> inputs and outputs. There does not seem to be any good reason to
> overwrite the values though.
> 
> Within vout_UpdateSourceCrop(), the value should actually not change
> (much like the aspect ratio numerator and denominator in the previous
> patchset), except for rounding errors and out-of-bound results.
> 
> The values are used in only one other function, vout_SetDisplayCrop().
> There, old and new values are compared to avoid needlessly resetting
> the display. Those comparisons actually work correctly only if the
> values are *not* modified by vout_UpdateSourceCrop().
> ---
>   src/video_output/display.c | 10 +---------
>   1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/src/video_output/display.c b/src/video_output/display.c
> index 06fb16ffc1..6d91d57849 100644
> --- a/src/video_output/display.c
> +++ b/src/video_output/display.c
> @@ -495,15 +495,7 @@ static int vout_UpdateSourceCrop(vout_display_t *vd)
>       video_format_Print(VLC_OBJECT(vd), "SOURCE ", &fmt);
>       video_format_Print(VLC_OBJECT(vd), "CROPPED ", &osys->source);
>   
> -    int ret = vout_display_Control(vd, VOUT_DISPLAY_CHANGE_SOURCE_CROP);
> -    osys->crop.left   = left - osys->source.i_x_offset;
> -    osys->crop.top    = top  - osys->source.i_y_offset;
> -    /* FIXME for right/bottom we should keep the 'type' border vs window */
> -    osys->crop.right  = right -
> -                        (osys->source.i_x_offset + osys->source.i_visible_width);
> -    osys->crop.bottom = bottom -
> -                        (osys->source.i_y_offset + osys->source.i_visible_height);

This sets back the values to 0 (since osys->source.i_x_offset = left and 
the other values are similar) and vd->source (aka osys->source) is 
read-only for the display module.

If the code is next called with vout_UpdateDisplaySourceProperties() 
osys->crop.left (and the others) will be 0. This effectively resets a 
previous call to vout_SetDisplayCrop(). I don't know if it's intentional 
or not. I don't think there's a good way to handle this case: the user 
set a manual crop on certain video dimension and then the video 
dimension changes.

Apart from this change it seems safe not to touch/reset these values 
which should be set whenever a new "crop" type+value is set. (I didn't 
check with your previous changes, only the current master).


> -    return ret;
> +    return vout_display_Control(vd, VOUT_DISPLAY_CHANGE_SOURCE_CROP);
>   }
>   
>   static int vout_SetSourceAspect(vout_display_t *vd,
> -- 
> 2.30.0
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>