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

[Stephen Warren]

> Reimar Döffinger
> 
> > +static void calc_drwXY_dWH(uint32_t *drwX, uint32_t *drwY,\
> > uint32_t *d_wh, uint32_t *d_ht)
> > +{
> > +    vo_calc_drwXY(drwX, drwY);
> > +
> > +    outRectVid.x0 = *drwX;
> > +    outRectVid.y0 = *drwY;
> > +    outRectVid.x1 = vo_dwidth+outRectVid.x0;
> > +    outRectVid.y1 = vo_dheight+outRectVid.y0;
> > +
> > +    outRect.x0 = 0;
> > +    outRect.x1 = FFMAX(vo_screenwidth, outRectVid.x1);
> > +    outRect.y0 = 0;
> > +    outRect.y1 = FFMAX(vo_screenheight, outRectVid.y1);
> 
> Are these FFMAX indeed necessary? What for exactly? vo_screenwidth
> may be unreliable. How does this handle videos larger than the
> screen resolution (in non-fullscreen mode, so the video "hangs out
> of the screen"?

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.

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

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

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

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. The YV12 support is for
the latter. I'm not convinced that removing it would be a great
idea unless it were to be added back immediately after, which seems
a little pointless in terms of busy work. However, I guess I see
the value of integrating code in small chunks.

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

-- 
nvpublic