[MPlayer-dev-eng] [PATCH]VO_VDPAU, round 5

[Reimar Döffinger]

On Thu, Jan 22, 2009 at 08:42:22AM -0800, Stephen Warren wrote:
> If I Recall Correctly, We put those in explicitly so that videos
> wouldn't hang off the screen.
> 
> We attempted to make the behavior of vo_vdpau exactly match vo_xv as
> much as possible, and I believe this change was made as part of that
> effort.

If it behaves as I expect, I think you might have implemented a
bug/deficiency in xv :-).
I am of course biased, but I'd consider -vo gl to be "authoritative"
for correct behaviour because it is the best-maintained...

> > > +    vdp_st = vdp_device_create_x11(mDisplay, mScreen,\
> > &vdp_device, &vdp_get_proc_address);
> > > +    if (vdp_st != VDP_STATUS_OK)
> > > +        mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> > > +               vdp_st, __FILE__, __LINE__);
> > 
> > I imagine saying it before: if these error checks are always the
> > same, make them a macro.
> 
> May I point out that earlier, this used to be a macro, but somebody
> specifically requested not to obfuscate the code with macros!

Yes, but I also gave reasons:
1) the macro did the wrong thing exit() IIRC
2) the macro basically was an excuse to do the checks quick and dirty
instead of giving a proper output

What should be done, and what I actually meant to say:
where it makes sense, replace the messages by something user-readable,
fix the MSGL to make sense for the specific type of error,
fix the error handling (using return -1 or just ignoring or whatever,
which should fit the chosen MSGL).
If then there is still something identical, macros should be used as
appropriate.
I admit that "if these error checks are always the same, make them a
macro" was a bad way to say that.
Or to say it in another way: I objected to the macro because it lead to
a bad implementation. I expected that after implementing it properly,
a macro would no longer make sense. If it still does make sense it
should of course be used.

> > > +    /* -----VDPAU related code here -------- */
> > > +    if (num_video_surfaces)
> > > +        videoSurfaces = calloc(num_video_surfaces,\
> > sizeof(VdpVideoSurface));
> > 
> > You will also have to think how to handle this on multiple config()
> > calls
> 
> Hmmm. FYI, we weren't aware of -fixed-vo when we wrote the original
> vo_vdpau.c, so it undoubtedly won't handle that case correctly.

Well, you at least did check for vo_config_count, so ++ on making an
effort to do the right thing ;-)

> > > +    switch (image_format) {
> > > +        case IMGFMT_YV12:
> > > +            assert(mpi->num_planes == 3);
> > > +            vdp_ycbcr_format = VDP_YCBCR_FORMAT_YV12;
> > > +            destdata[0] = mpi->planes[0];
> > > +            destdata[1] = mpi->planes[2];
> > > +            destdata[2] = mpi->planes[1];
> > > +            videoSurface = videoSurfaces[0];
> > > +
> > > ...
> > > +        default:
> > > +            break;
> > > +    }
> > 
> > I don't really like this, a switch just for two cases seems
> > overkill, also it is unclear to me if this code just to support
> > outputnof normal YV12 data, if so it might make sense to cut the
> > size by removing support for that at least for now (as long as
> > there is no support for post-processing it is not really useful
> > anyway).
> 
> We originally conceived support for other surface types too, but
> only got around to implementing YV12 fully. Hence, there could be
> more case entries later.

Yes, but you are adding complexity now with the expectation that a
switch will be the most effective way to add additional formats later
will be via a switch instead of e.g. some clever tricks that avoids code
duplication better.
IMHO planning for future on the code level hardly ever works well and
should be restricted to the overall design.

> There are two paths overall through vo_vdpau; HW accelerated
> decoding using VDPAU, and SW decoding, just using vo_vdpau as the
> display path, just like e.g. vo_xv/vo_x11.

I understand that, and I do not really want to remove it, I only meant
this as another option to avoid reviewing/cleaning it up right now.
An additional point was, that it seems a rather pointless feature right
now, we have enough vos to display software decoding, so conceptually it
might make more sense to implement this once support for post-processing
features is added.

> > > +    if (!num_video_surfaces) { // RGB surface formats
> > > +        switch (image_format) {
> > > +            case IMGFMT_BGRA:
> > 
> > There is a comparison against the num_video_surfaces variable and
> > one direct after for IMGFMT_BGRA, and the former one carries the
> > comment "// RGB surface formats". To me that is always an
> > indication that the code makes no sense and you are checking for
> > the wrong thing,
> 
> Given the "!num_video_surfaces" check, I think the comment should
> really be "SW decoding" or "Non-HW-accelerated decoding" or
> "Non VDPAU surface formats".

Hm, does this mean that post-processing features will not be possible to
use on RGBA? If so, would it in theory be possible to add
post-processing support for it and would then probably the code have to
be changed (to e.g. be more similar to the YV12 path)?
Not sure if it really matters for the current code so I don't really
need an answer, just if you happen to have an opinion (I don't expect
you to predict the future, just an educated guess!) on this...

Greetings,
Reimar Döffinger