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

[Xidorn Quan]

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

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

Not a bad idea, I'll use this.

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

So, is it ok not to change this code?


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

I agree with you that logically, outfmt should be set in config
instead of in vf_open. Here I'm just following the method the origin
code is using. Fixing this logical problem is actually unrelated with
this patch, though it's correct. We can fix it in a further patch
which is attempted to do so.

BTW, there is a same logical problem in vo_corevideo, and many other
places maybe.