[vlc-devel] [PATCH] core: do not overwrite visible size

[Felix Abecassis]

We agreed on IRC that this doesn't look like a bug. The swscale
context is initialized properly and the scaling function need the
actual stride of the picture plane in order to move to the next line.

However I think this double padding mechanism in the code is
confusing. It might be better to use visible and hidden resolution
only for cropping.
The whole rectangle defined by (0, 0), (i_width, i_height) should have
valid pixels but only (i_x_offset, i_y_offset), (i_x_offset +
i_visible_width, i_y_offset + i_visible_height) should be displayed.
If a filter is not using the visible resolution then it would be doing
wasteful computations but it should not be accessing uninitialized
pixels (however it might be incorrect if the filter need to compute
the center of the picture for instance). This is currently an issue
with swscale since it is using i_width/i_heigth and thus is using the
uninitialized pixels from the avcodec padding; also
video_format_ScaleCropAr is messing up the dimensions of the picture
because of the padding.

If we enforce that, we need a way for modules like avcodec or Theora
to change the allocation alignment/padding, it doesn't look possible
right now.

What do you think? Are there use cases where this change would break the code?


2013/11/25 Felix Abecassis <felix.abecassis at gmail.com>:
> In your patch:
> +    msg_Err(p_filter, "i_height=%i", i_height);
> +    plane_t *p = &(p_src->p[0]);
> +    msg_Err(p_filter, "src %ix%i (%ix%i)",
> +            p->i_pitch / p->i_pixel_pitch, p->i_lines,
> +            p->i_visible_pitch / p->i_pixel_pitch, p->i_visible_lines);
> +    p = &(p_dst->p[0]);
> +    msg_Err(p_filter, "dst %ix%i (%ix%i)",
> +            p->i_pitch / p->i_pixel_pitch, p->i_lines,
> +            p->i_visible_pitch / p->i_pixel_pitch, p->i_visible_lines);
>
> Why are you printing these values? Picture allocation seems to have
> its own padding code. In picture_Setup: (src/misc/picture.c)
> const int i_width_aligned  = ( i_width  + i_modulo_w - 1 ) /
> i_modulo_w * i_modulo_w;
> const int i_height_aligned = ( i_height + i_modulo_h - 1 ) /
> i_modulo_h * i_modulo_h;
> const int i_height_extra   = 2 * i_ratio_h; /* This one is a hack for
> some ASM functions */
>
> Where i_modulo_w and i_modulo_h are computed using the chroma type of
> the picture.
>
> So we have two levels of padding in the code:
> - Plane allocation: i_visible_pitch/i_visible_lines vs
> i_pitch/i_lines. Because of the above code, avcodec DR is still
> enabled even if we don't call avcodec_align_dimensions2 (but we
> shouldn't rely on that obviously).
> - Picture: i_visible_width/i_visible_height vs i_width/i_height. Using
> also i_x_offset/i_y_offset this can be used to implement cropping. But
> it's also used for padding (avcodec/Theora modules).
> Since it looks like we cannot influence the alignment used during
> plane allocation we have to use this instead.
>
> In function Convert, p_src.format and p_dst.format seems to be
> consistent with the resolution swscale was configured with.



-- 
Félix Abecassis
http://felix.abecassis.me