[FFmpeg-devel] [PATCH]VDPAU support for MPEG-4 ASP

[Reimar Döffinger]

On Sat, Sep 05, 2009 at 12:16:14PM +0200, Carl Eugen Hoyos wrote:
> On Saturday 05 September 2009 10:17:08 Reimar D?ffinger wrote:
> > On Sat, Sep 05, 2009 at 01:08:46AM +0200, Carl Eugen Hoyos wrote:
> > > Attached patch adds support for decoding MPEG-4 ASP on Nvidia's latest
> > > VDPAU hardware.
> >
> > This brings again up the same question: why do we have an extra codec
> > for these formats when the acceleration could just be selected via
> > pixfmt?
> 
> How would this work when ffmpeg will support hardware accelerated decoding (I 
> mean the resulting pix_fmt would then be the same for software decoding and 
> hardware decoding)?

Why should you want to make selecting hardware accelerated decoding
extra-hard for applications? They can't use avcodec_find_decoder so they
must keep a manually updated list or scan all decoders in order to find
the accelerated one instead of e.g. just setting a flag in
AVCodecContext.

> > > +        int d = gcd(s->pp_time, s->pb_time);
> > > +        render->info.mpeg4.trd[0] = s->pp_time / d;
> > > +        render->info.mpeg4.trb[0] = s->pb_time / d;
> >
> > av_reduce?
> 
> Sorry, I don't understand how it would simplify the code?

Well, the name is more obvious. But apart from that not much of a point.

> > > +        render->info.mpeg4.trd[1] = (s->time - s->pb_time + s->pp_time
> > > +                                       + (s->pb_time >> 1))/s->pb_time
> > > +               - (s->time - s->pb_time + (s->pb_time >> 1))/s->pb_time;
> > > +        render->info.mpeg4.trb[1] = (s->time + (s->pb_time >>
> > > 1))/s->pb_time +                     - (s->time - s->pb_time +
> > > (s->pb_time >> 1))/s->pb_time;
> >
> > Is it really necessary to round and divide twice?
> > At least this needs an explaining comment.
> 
> Nvidia's original version looks like this:
>         cur_time = s->time;
>         fwd_time = cur_time - s->pb_time;
>         bwd_time = fwd_time +  s->pp_time;
>         frame_time = cur_time - fwd_time;
> 
>         trd[0] =  s->pp_time;
>         trb[0] =  s->pb_time;
>         d = gcd(trb[0], trd[0]);
>         trb[0] /= d;
>         trd[0] /= d;
> 
>         trd[1] = (bwd_time + (frame_time >> 1))/frame_time -
>                               (fwd_time + (frame_time >> 1))/frame_time;
>         trb[1] = (cur_time + (frame_time >> 1))/frame_time -
>                               (fwd_time + (frame_time >> 1))/frame_time;
> 
> I hoped my version is a little simpler.

Maybe, but what is wrong with
render->info.mpeg4.trd[1] = (s->pp_time + (s->pb_time >> 1)) / s->pb_time;
render->info.mpeg4.trb[1] = (s->pb_time + (s->pb_time >> 1)) / s->pb_time;
It is not the same thing but the questions is if and why it matters, and
that needs to be a comment.
Completely ignoring that coming up with names like trd and trb as part
of a public API nearly justifies a public flogging.