[MPlayer-dev-eng] Mplayer -vo ps3

[Reimar Döffinger]

On Sat, Apr 04, 2009 at 10:26:33PM +0200, Kristian Jerpetjøn wrote:
> >> +/* preinit - pre-initialization section
> >> +*
> >> +* Called before initializing video output driver for evaluating subopts.
> >> +* Currently, we don't utilize any subopts (e.g. -vo ps3:subopt
> >> +*/
> >
> > Comment is outdated, it should be doxygen compatible, but documenting functions
> > that are part of the standard VO API is a bit pointless.
> 
> Working on it throughout file
>  This style is correct / ok ?
> 
> /**
> * Pointless test function
> *
> * @param testval test value for testfunct
> * @return returns the test val
> */
> static int testfuct(int testval){
>     return testval;
> }

Yes, looks right to me.

> >> +            return VFCAP_CSP_SUPPORTED | VFCAP_CSP_SUPPORTED_BY_HW | VFCAP_SWSCALE | VFCAP_ACCEPT_STRIDE;
> >
> > not sure VFCAP_SWSCALE is right, at least you are not using libswscale...
> 
> removed seems to work fine without

I am just not sure if it should actually set the HWSCALE flag or what it is
called. Sure, it is in software, but with recent GPUs OpenGL etc. is just
software too, the point is that it does not use the "main CPU".
But as said, I am not completely sure myself.

> >> +    vo_doublebuffering = VO_TRUE;
> >> +    vo_directrendering = VO_TRUE;
> >
> > These really should be left to the user and not overridden.
> 
> removed now generates uglyness if -dr is disabled from user

Yes, I think I started understanding what the code does, hopefully
I can explain to you what goes wrong and why.

> I need a serious amount of help here..
> changing MP_IMGFLAG_ALLOCATED to MP_IMGFLAG_DIRECT gives me green/tearing output

Yeah, it looks like "a load of bugs which all together almost cancel each other out"

> >> +static int draw_frame ( uint8_t *src[] )
> >> +{
> >> +    draw_frame_count++;
> >> +
> >> +    return draw_slice ( src, src_stride, srcW, srcH, 0, 0 );
> >> +}
> >
> > There is no need and no point in implementing this if you have draw_image.
> 
> function needed by prototype ? compilation issues when i remove it
> function now does nothing things works fine.

Yes, that's what I meant by "not implementing".

> >> +        case VOCTRL_UPDATE_SCREENINFO: /* 32 */
> >> +            mp_msg ( MSGT_VO, MSGL_WARN, "[vo_ps3] control: todo: handle UPDATE_SCREENINFO (%u)\n", request );
> >> +            return VO_NOTIMPL;
> >
> > can't you just set xinerama_x, xinerama_y, vo_screenwidth and vo_screenheight?
> > xinerama_? to 0, the others to the framebuffer dimension (not this is called
> > before config but after preinit).
> 
> need to understand implications of this with respect to current code
> before attempting

Basically it just preinitializes vo_dx/vo_dy/vo_dwidth etc. and sets up
aspect stuff. Should just allow you to remove a lot of code but not change much.

> >> +    vo_dx = vo_dy = 0;
> >> +    vo_screenwidth=fbW - ( fbX*2 );
> >> +    vo_screenheight=fbH - ( fbY*2 );
> > [...]
> >> +    //set aspect stuff
> >> +    aspect_save_orig ( srcW, srcH );
> >> +    aspect_save_screenres ( vo_screenwidth, vo_screenheight );
> >> +    aspect_save_prescale ( dstW, dstH );
> >> +
> >> +    geometry ( &vo_dx, &vo_dy, &dstW, &dstH, vo_screenwidth, vo_screenheight );
> >> +    aspect ( &dstW, &dstH, A_NOZOOM );
> >> +    //center image -- this is wrong from mplayer's perspective - it should be
> >> +    //                before the geometry & aspect call, but i don't want
> >> +    //                mplayer to center the image because i can do it in
> >> +    //                spu-medialib and we don't have to draw the extra black bars
> >> +    vo_dx = ( fbW - ( 2 * fbX ) - dstW ) /2;
> >> +    vo_dx = ( vo_dx+3 ) &~3;
> >> +    vo_dy = ( fbH - ( 2 * fbY ) - dstH ) /2;
> >
> > Implement VOCTRL_UPDATE_SCREENINFO and you (hopefully) shouldn't need any of that.
> > I have no idea WTF that comment is talking about.
> > I guess the code was never tested with -geometry though...
> 
> need some help implementing geometry correctly
> however i did some cleanup here

Well, the idea is: just implement VOCTRL_UPDATE_SCREENINFO and then display
the video at vo_dx, vo_dy with size vo_dwidth, vo_dheight.
To go fullscreen remember these and set the size from aspect(..., A_ZOOM) and
vo_dx, vo_dy to create proper black borders.
To return from fullscreen, restore the previous values.

> >> +    yuvscsc_set_srcW ( yuvcsc, srcW );
> >> +    yuvscsc_set_srcH ( yuvcsc, srcH );
> >> +    yuvscsc_set_dstW ( yuvcsc, dstW );
> >> +    yuvscsc_set_dstH ( yuvcsc, dstH );
> >> +    yuvscsc_set_offset ( yuvcsc, offset );
> >> +    yuvscsc_set_maxwidth ( yuvcsc, maxW );
> >
> > Somebody has not yet found out about the thing called "struct" yet?!?
> 
> Yes we have but unfortunately i have not bothered to make a new API
> for this part of spu-medialib yet i should put it on the todo for 0.2

It's not that bad, I just considered it a bit funny and coould not resist a comment.

> It is one step closer now and i need help on a few parts regarding the
> get_image and draw_image and general buffer handling
> 
> Currently spu-medialib is configured with two planar images as input
> divided amongst 6 buffers for yv12 etc...

Two buffers is not going to great with -dr.
For it to work well with MPEG-2 you need one to store the I-frame in,
one to store the P-frame in, and then you need to draw the B-frame
without break the I- or P-frame.

> +static uint32_t get_image ( mp_image_t * mpi )
> +{
> +    //TODO fix this whole mess somehow
> +    get_image_count++;
> +
> +    if ( ( mpi->flags & MP_IMGFLAG_READABLE ) && ( mpi->type == MP_IMGTYPE_IPB\
> +        || mpi->type == MP_IMGTYPE_IP ) )

The \ is not necessary.

> +    {
> +        buf_plane_ptr[0] = parking_lot_y[parking_lot_page];
> +        buf_plane_ptr[1] = parking_lot_u[parking_lot_page];
> +        buf_plane_ptr[2] = parking_lot_v[parking_lot_page];
> +        //mpi->flags |=MP_IMGFLAG_DIRECT;
> +        //FIXME crash without understand what Bill was thinking here
> +        mpi->flags |= MP_IMGFLAG_ALLOCATED;
> +        parking_lot_page^=1;
> +    }
> +    else
> +    {
> +        buf_plane_ptr[0] = inbuf_y[page];
> +        buf_plane_ptr[1] = inbuf_u[page];
> +        buf_plane_ptr[2] = inbuf_v[page];
> +        mpi->flags |= MP_IMGFLAG_DIRECT;
> +    }

If you do not set MP_IMGFLAG_DIRECT that means in draw_image you will
still call memcpy, thus get_image is completely pointless then, so just
returning VO_FALSE in that case should work the same.
Next, you have only two buffers available. You need one in case you
somehow get and mpi without MP_IMGFLAG_DIRECT in draw_image.
Since you do not know when that will happen, you can not give that one
out in get_image when MP_IMGFLAG_PRESERVE is set (i.e. the application
expects to get the same content back somewhen).
Now I am not sure if the second buffer is available or if that one might
still be processed by the SPEs.
Either way, that one buffer is not enought for MP_IMGTYPE_IP.
So you end up with two possibilities:
1)
  if (mpi->flags & MP_IMGFLAG_PRESERVE) return VO_FALSE;
  if (mpi->type != MP_IMGTYPE_STATIC && mpi->type != MP_IMGTYPE_TEMP &&
      (mpi->type != MP_IMGTYPE_NUMBERED || mpi->number))
    return VO_FALSE;
  fill planes etc. with the same buffer you would use in draw_image and
  set MP_IMGFLAG_DIRECT, skip the memcpy in draw_image if that flag is set

2)
  if (mpi->type != MP_IMGTYPE_STATIC && mpi->type != MP_IMGTYPE_TEMP &&
      (mpi->type != MP_IMGTYPE_NUMBERED || mpi->number))
    return VO_FALSE;
  fill planes etc. with the second buffer and set MP_IMGFLAG_DIRECT, skip
  the memcpy in draw_image if that flag is set.
  If that flag is not set in memcpy, memcpy _always_ to the first buffer
  in draw_image.

Either way unless you provide at least 3 buffers (and ignore correctness a bit,
which is okay since vo_xv does so already) or better 4 buffers, you will
probably gain little from direct-rendering...
btw. you should just call draw_slice to make the memcpy in draw_image,
if they do not use the same code they are almost certainly wrong :-)

> +static void draw_osd ( void )
> +{
> +
> +    vo_draw_text ( srcW, srcH, draw_alpha );
> +}

Hm, I somehow missed it, what does draw_alpha draw onto?
Hopefully not onto something you gave out as PRESERVE/READABLE in get_image
because then you'd just have broken a promise :-)

> +    goto complete;
> +
> +framebuffer:
> +    if (fb >= 0){
> +        ioctl ( fb,PS3FB_IOCTL_OFF );
> +        close ( fb );
> +    }
> +
> +console:
> +    if (console >= 0) {
> +
> +        ioctl ( console,KDSETMODE,KD_TEXT );
> +        close(console);
> +    }
> +
> +complete:
> +    return res;

Nah, that's the evil "spaghetti code" way of using goto.
Do it like this:

fb = -1;
console = -1;
...
if (something went wrong)
    goto fail;
...
return 0;

fail:
if (fb >= 0) ...
if (console >= 0)
return -1;