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

[Xidorn Quan]

On Sat, Aug 25, 2012 at 7:18 PM, Reimar Döffinger
<Reimar.Doeffinger at gmx.de>wrote:

> 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).
>

I'll change this, but do you have any suggestion on how to name
these types?

> +    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?
>

The flow is that, function of type copy_from_image converts any image
to pixel format yuv444p in its inner buffer, then common function
my_draw_bitmap renders ass on pictures, and finally copy_to_image
converts yuv444p in the inner buffer back to the origin pixel format.

For planar formats, copy_from_image just upsampling u and v planar to
its inner buffer, but for non-planar formats, we have to copy the y
to planar[0] at the same time as well. So I alloc all planars 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.
>

OK. I just thought that the code have been wrapped more deeply so
I reindent them. If it isn't allowed, I won't do this again.

> +    } 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);
>

I'll fix this.

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

It's my fault. I shouldn't change this.

> +    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
>

a) dst_stride is exactly equal to the output width. Just several
lines above, you can find dst_stride = vf->priv->outw.
b) for the same reason above it can't, or width can be negative, too.


> > +                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)
>

The origin code which work on 422p do the same way, well, except
that it uses vf->dmpi->chroma_width instead of simply width.
Is this the correct solution?

> -    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.
>

It is not unrelated. This vf is designed to output pictures with
the same format with input one so it can do converting as few as
possible (since subtitles usually don't cover a large area).

This code should be here or it will go wrong.

> +    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?
>

It's my fault. I'll fix this.

Thanks,
Xidorn Quan