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

[Reimar Döffinger]

On Sat, Aug 25, 2012 at 08:45:23PM +0800, Xidorn Quan wrote:
> 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?

Not particularly, but copy_from_image_func maybe?

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

I probably then missed the overall picture here, I assumed it to be
an ordinary mp_image stride.

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

I admit I don't know, I agree that it makes no sense to do it
differently between the two cases.

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

Well, but that does not have anything to do with supporting
YUY2/UYVY formats but would make sense even without it?
I think that whole logic is broken, vf->priv->outfmt should
not be decided on opening the filter, but it should depend on the
input format.
This also means that the query function then can't use vf->priv->outfmt
at all (because it will not me set) but instead it should return
vf_next_query_format(vf, fmt) for all fmt it supports just like other
filters.
In addition it should be adding VFCAP_EOSD to the flags, since it
provides that.
The only issue with all this is that it still needs to do a bit
of a hack to check for VFCAP_EOSD on vf_open.