[MPlayer-dev-eng] vo_gl PBO patch ..

[Reimar Döffinger]

On Tue, Apr 29, 2008 at 11:51:20AM -0600, Sven Gothel wrote:
> fixing a minor possible bug in case bytes-per-pixel!=1
> in gl_common.c
> 
> btw .. would anybody like to review this ?
> thank you.

I have very little time right now, but I will look at it (this
is not a complete review).
I do quite dislike the code duplication in the draw_slice function
though, and it is quite unclear which part of the changes make the
biggest difference, e.g. if implementing MP_IMGTYPE_IP and
MP_IMGTYPE_IPB in get_image would be enough.
Also the patch changing mp_msg calls at the same time and converting
existing code to use the gl...PBO wrapper functions (which so far seem
a bit like overkill to me anyway) make this patch quite hard to
understand.
In addition, I have some doubts if using more than one PBO is a good
idea.

> @@ -646,16 +666,30 @@
>  static int draw_slice(uint8_t *src[], int stride[], int w,int h,int x,int y)
>  {
>    mpi_flipped = (stride[0] < 0);
> -  glUploadTex(gl_target, gl_format, gl_type, src[0], stride[0],
> -              x, y, w, h, slice_height);
> -  if (image_format == IMGFMT_YV12) {
> -    ActiveTexture(GL_TEXTURE1);
> -    glUploadTex(gl_target, gl_format, gl_type, src[1], stride[1],
> -                x / 2, y / 2, w / 2, h / 2, slice_height);
> -    ActiveTexture(GL_TEXTURE2);
> -    glUploadTex(gl_target, gl_format, gl_type, src[2], stride[2],
> -                x / 2, y / 2, w / 2, h / 2, slice_height);
> -    ActiveTexture(GL_TEXTURE0);
> +  if(!use_pboDMA) {
> +      glUploadTex(gl_target, gl_format, gl_type, src[0], stride[0],
> +                  x, y, w, h, slice_height);
> +      if (image_format == IMGFMT_YV12) {
> +        ActiveTexture(GL_TEXTURE1);
> +        glUploadTex(gl_target, gl_format, gl_type, src[1], stride[1],
> +                    x / 2, y / 2, w / 2, h / 2, slice_height);
> +        ActiveTexture(GL_TEXTURE2);
> +        glUploadTex(gl_target, gl_format, gl_type, src[2], stride[2],
> +                    x / 2, y / 2, w / 2, h / 2, slice_height);
> +        ActiveTexture(GL_TEXTURE0);
> +      }

Reindentation (aka "cosmetics") may not be mixed with functional
changes.

> @@ -882,6 +949,17 @@
>                 "Use -vo gl:nomanyfmts if playback fails.\n");
>      mp_msg (MSGT_VO, MSGL_V, "[gl] Using %d as slice height "
>               "(0 means image height).\n", slice_height);
> +
> +    switch(use_pboDMA) {
> +        case  1:
> +            gl_pboDMA = PBO_XFER_MULTIPLE_MEMCPY;
> +            break;
> +        case  2:
> +            gl_pboDMA = PBO_XFER_SINGLE_MEMCPY;
> +            break;
> +        default:
> +            gl_pboDMA = PBO_XFER_DISABLED;
> +    }

Decoupling UI and internal code is a good idea, but it also bloats the
code. I would prefer if the user would just set gl_pboDMA directly.
If ever a proper separation is desired, it would be better to extend
the parser to support it directly.

> +  if ( pbo->mode==PBO_XFER_SINGLE_MEMCPY && h * stride > pbo->sz ) {
> +    pbo->mode=PBO_XFER_MULTIPLE_MEMCPY;
> +    mp_msg (MSGT_VO, MSGL_INFO, "[gl] glUploadTexPBO %d: PBO single -> multiple memcpy (video stride)!\n", 
> +        pbo->name);

If you intend to check if there is a stride applied, "h * stride >
pbo->sz" is the wrong condition, "stride > w * bytes_per_pixel" is the
right one.

> +  if ( h * w > pbo->sz )
> +  {
> +    mp_msg (MSGT_VO, MSGL_INFO, "[gl] glUploadTexPBO %d: %d/%d %dx%d, sz %d, slic %d, strd %d, bpp %d -> PBO Disabled (video size)!\n", 
> +      pbo->name, x,y, w,h,  pbo->sz, slice, stride, bytesPerPixel);
> +    glDestroyPBO(pbo);
> +
> +    // fallback ..
> +    glUploadTex(target, format, type, dataptr, stride, x, y, w, h, slice);
> +    return;
> +  }

Either this is possible, then it should be supported properly (not just
via fallback) or it should just not do anything, according to the motto:
better fail hard than continue with an unexpected performance
degradation - users are unlikely to notice the message, failing will
ensure we will get a bug report instead of a user just thinking "oh
MPlayer is so slow..." and never reporting it.

> +  if((pbo->test&1)==0) {
> +      mp_msg (MSGT_VO, MSGL_V, "[gl] glUploadTexPBO %d: %d/%d %dx%d, sz %d, slic %d, strd %d, bpp %d - mode %d\n", 
> +        pbo->name, x,y, w,h,  pbo->sz, slice, stride, bytesPerPixel, pbo->mode);
> +      pbo->test|=1;
> +  }

IMO that has no place in productive code.

> +  if(pbo->mode==PBO_XFER_SINGLE_MEMCPY) {
> +    memcpy(pbo->mem, data,  h*stride);
> +    rowlenBytes = stride;
> +  } else if(pbo->mode==PBO_XFER_MULTIPLE_MEMCPY) {
> +    for(i=0;i<h;i++)
> +    {
> +        memcpy(pbo->mem+(i*pbo->stride), data+(i*stride), w*bytesPerPixel);
> +    }

You should probably look at fast_memcpy, mem2agpcpy, memcpy_pic and
mem2agpcpy_pic

Greetings,
Reimar Döffinger