On Sat, 06 Sep 2014 18:10:17 +0300
Rémi Denis-Courmont <[email protected]> wrote:
> Hello,
>
> Le vendredi 5 septembre 2014, 18:13:49 wm4 a écrit :
> > > +static VdpStatus vdp_enosys(/* Unspecified, not void - leave empty */)
> > > +{
> > > + return VDP_STATUS_NO_IMPLEMENTATION;
> > > +}
> > > +
> > > +static void *ff_vdpau_get_proc_address(AVCodecContext *avctx, VdpFuncId
> > > id) +{
> > > + AVVDPAUContext *hwctx = avctx->hwaccel_context;
> > > + void *ret;
> > > +
> > > + av_assert0(hwctx->device != VDP_INVALID_HANDLE);
> > > +
> > > + if (hwctx->get_proc_address(hwctx->device, id, &ret) !=
> > > VDP_STATUS_OK)
> > > + ret = vdp_enosys;
> >
> > Isn't this very questionable? IMO initialization should just fail if a
> > required function is missing. But if a C language lawyer can confirm
> > that this use of vdp_enosys is indeed legal, why not (although I
> > have my doubt).
>
> I think vdp_enosys is legal thanks to the C calling convention (i.e. the
> called function does not need to know its parameters - this is necessary for
> variadic functions). I can change it to NULL but it will mostly result in
> more
> extra error handling boilerplate - only to handle completely broken VDPAU
> drivers (of which I know none).
>
> For such hypothetically broken driver, I think not failing safe is good
> enough.
No, I'm pretty sure it's invalid C. Casting function types is
legal, but dereferencing (i.e. calling) a function pointer is legal
only if the actual types match.
Maybe use some macros to define the function tables and the code to
load it.
Well, I don't care that much - even if invalid, it's unlikely that
compilers suddenly start caring, and it miscompilation happens, we
still can fix it later. So no blocker.
> > The idea seems generally ok (and making libavcodec handle more aspects
> > of hw decoding is welcome). But I have some doubts whether it will
> > still be possible for me to handle preemption as well as I could, and I
> > wouldn't appreciate if a small incremental improvement breaks all of my
> > code.
>
> Unless you have a _concrete_ enhancement request before next Tuesday, I will
> go ahead and try to address Diego and any further feedback. I am definitely
> not
> waiting an indefinite amount of time for the sake of an abstract request, and
> potentially holding up new codec support. Plus I would like to fix this
> before
> VLC 2.2.0.
>
> Specifically as regards preemption, I think it remains doable. In principles,
> this makes no real difference; you can still wrap the callback in a similarly
> inconvenient manner as before and with the same limitations.
Have you actually implemented it? I don't feel like having to update my
code to an incrementally changing API that doesn't even consider this
use case. Maybe implement preemption recovery in VLC (AFAIK VLC
doesn't handle it yet?). Then we could come up with something
reasonable, instead of creating theories or declaring it as broken.
Now I'm not even against your patches. I just hate it figuring out this
preemption crap again every time. I don't actually like preemption, but
handling it is kind of needed.
> I think however that you have already demonstrated that preemption needs
> fixing
> at the back-end. But the venue for that discussion is the VDPAU FreeDesktop
> list, not libav-devel. Regardless, this patch does not increase the risk of
> needing yet another API change for preemption for those apps that do care; I
> don't see why yet another interface break would be needed either way.
>
> > Also, think of the API users: do they have to update their vdpau code
> > every month now? WTF?
>
> I fail to see how this forces anybody to update. This should be backward
> compatible with the previous three variants of the VDPAU hardware
> acceleration. Once a month? obvious troll is obvious.
But it's declared as deprecated. The next major bump will disable it.
> Of course, backward compatibility will break if/when new chroma types are
> implemented, e.g. Hi10p. Sorry but I *told* *you* *so*. IIRC, you were among
> the developers to object to different pixel formats for different chroma
> types.
> I can still make that change if people changed their mind.
Against pixel formats - not necessarily against a mechanism to set the
chroma type. AFAIR we were worried about the possible outgrowth
of additional pixel formats. For example, doesn't DXVA use FourCCs to
describe surface formats? That would be hard to manage.
Anyway, this is somewhat orthogonal to this patch, isn't it?
> > A solution to this might be not deprecating the "old" API. Then
> > downstreams can wait it out until the API finally becomes sane after a
> > bunch of more incremental improvements. This is true for the other
> > hwaccels as well (since I always hear of hwaccel 1.x...).
>
> The whole point of the series is to insulate applications from:
> - new codecs, profiles and levels (for real),
> - new chroma types,
> - new function calls (as far as possible).
>
> If we postpone the change, all VDPAU-enabled applications will need updates
> for HEVC then Hi10 and who-knows-what later. That seems like a worse
> alternative on all counts.
These applications need a change anyway. Why can't this change wait
until HEVC and Hi10 (if the latter ever happens) are set in stone
API-wise?
> My NVIDIA colleagues and I would also prefer to have more exemplary code in
> libavcodec. I cannot say that I am satisfied with the current state of
> affairs.
> In fact, I wanted something like this patch from the beginning. Only at the
> time, that was not yet possible for lack of init, uninit and priv_data_size.
>
NVIDIA should probably work on making preemption reasonably
implementable (preferably by deprecating and removing it), so we don't
have such problems in the first place.
Anyway, I guess I can try this patch and see how it interacts with my
implementation of preemption and if there are any actual problems. But
not today.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
