[MPlayer-dev-eng] [PATCH] vf_ass: support uyvy and yuy2 directly

[Reimar Döffinger]

On Thu, Aug 23, 2012 at 11:28:55PM +0800, Xidorn Quan wrote:
> Hi,
> 
> This patch adds uyvy and yuy2 format support for vf_ass. uyvy and yuy2
> pictures then needn't to be convert by scale anymore when using ass.
> 
> Though, I think the code can probably be optimized more by using sth
> like MMX, or considering more about CPU cache. But I'm not experienced
> in doing so.
> 
> Best Regards,
> Xidorn Quan

> +typedef void (*copy_from_image_t)(struct vf_instance *vf, int first_row, int last_row);
> +typedef void (*copy_to_image_t)(struct vf_instance *vf);

Names ending with _t are reserved by POSIX, so we shouldn't really use
them (even if we already do in a lot of places I admit).

> +    if (!vf->priv->is_planar)
> +        vf->priv->planes[0] = malloc(vf->priv->outw * vf->priv->outh);
>      vf->priv->planes[1]  = malloc(vf->priv->outw * vf->priv->outh);
>      vf->priv->planes[2]  = malloc(vf->priv->outw * vf->priv->outh);

Why are you allocating data for planes[1] and [2] for non-planar
formats?

> -    dst = mpi->planes[0] + y1 * mpi->stride[0];
> -    for (y = 0; y < y2 - y1; ++y) {
> -        memset(dst, color[0], mpi->w);
> -        dst += mpi->stride[0];
> +    if (mpi->flags & MP_IMGFLAG_PLANAR) {
> +        dst = mpi->planes[0] + y1 * mpi->stride[0];
> +        for (y = 0; y < y2 - y1; ++y) {
> +            memset(dst, color[0], mpi->w);
> +            dst += mpi->stride[0];

Please don't reindent code you did not change (or use the diff options
to ignore whitespace-only differences), it makes changes much easier to
review.

> +    } else {
> +        if (mpi->imgfmt == IMGFMT_UYVY) {
> +            packed_color.c[0] = color[1];
> +            packed_color.c[1] = color[0];
> +            packed_color.c[2] = color[2];
> +            packed_color.c[3] = color[0];
> +        } else if (mpi->imgfmt == IMGFMT_YUY2) {
> +            packed_color.c[0] = color[0];
> +            packed_color.c[1] = color[1];
> +            packed_color.c[2] = color[0];
> +            packed_color.c[3] = color[2];
> +        }
> +        dst = mpi->planes[0] + y1 * mpi->stride[0];
> +        for (y = 0; y < y2 - y1; ++y)
> +            for (x = 0; x < mpi->w; ++x)
> +                *((uint32_t *)dst + x) = packed_color.i;

Raw type-casting like that is risky, also falling back to e.g.
YUY2 case in case imgfmt should be something unexpected is safer
coding than using uninitialized data.
So I think it would be simpler as something along the lines of
} else {
    uint8_t c[4];
    if (mpi->imgfmt == IMGFMT_UYVY) {
...
    } else {
...
    }
....
       AV_COPY32(dst + 4*x, c);


> -            if ((vf->priv->dirty_rows[i * 2] == 1)) {
> +            if (vf->priv->dirty_rows[i * 2]) {

Why?

> +    for (i = first_row; i < last_row; ++i) {
> +        int next_off = dst_off + dst_stride;
> +        if (!dirty_rows[i]) {
> +            if (is_uyvy) {
> +                for (j = dst_off, k = 0; j < next_off; j += 2, k += 4) {

I don't think this can work, because
a) the area between the width and stride length should be considered
reserved and not accessed
b) stride can be negative

> +                for (j = src_off, k = 0; j < next_off; j += 2, k += 4) {
> +                    dst[k    ] = AVERAGE(src[1][j], src[1][j + 1]);

If next_off is odd, [j + 1] will access data outside the allocated
memory.
Also chroma usually isn't located in-between the Y values, so taking the
average is likely _less_ correct that just taking src[1][j].
(note: OS X might not take the chroma sitting issue seriously anyway,
but if it will be wrong either way the simpler code is better)

> -    switch (fmt) {
> -    case IMGFMT_YV12:
> -    case IMGFMT_I420:
> -    case IMGFMT_IYUV:
> +    if (fmt == vf->priv->outfmt)
>          return vf_next_query_format(vf, vf->priv->outfmt);

This seems slightly unrelated.

> +    if (vf->priv->is_planar)
> +        free(vf->priv->planes[0]);

The malloc used the inverted condition I think?
Did you try running this under valgrind?