[MPlayer-dev-eng] Mplayer -vo ps3

[Kristian Jerpetjøn]

No comments on the below ?
cheers
kristian

2009/4/7 Kristian Jerpetjøn <kristian.jerpetjoen at gmail.com>:
> 2009/4/5 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
>> 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.
>
> Unsure seems to work without from what i managed to read out of
> vf_vo.c its fine
>
>>
>>> >> +    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.
>
> Solved Uglyness
>
>>
>>> 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"
>
> Think i solved this one as well
>
>>> >> +        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.
>
> Think i solved this one as well
>
>>> >> +    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.
>
> New implementation
>
>>
>>> >> +    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.
>
> Well now its gone..
>
>>
>>> 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.
>
> Solved buffers = 4 when doing DR..
>
>>
>>> +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 ) )
>>
>
> removed
>
>>
>>> +    {
>>> +        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 :-)
>
> 4 buffers seemed fine :)
>
>>
>>> +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 :-)
>
> I will not solve OSD in this patch
> However i have now solved a lot of issues and the overall quality is
> greatly improved
>
> optional and working -fs -double -dr -geometry i think
>
> I have to make a new release of spu-medialib but it was a neccesary
> step to make it work with 4 buffers that release is ready the day the
> patch is approved
>
> Cheers
> --
> Kristian Jerpetjøn
> Tlf:        +4721694436
> Mob      +4792822774
> Email:  kristian.jerpetjoen at gmail.com
>



-- 
Kristian Jerpetjøn
Tlf:        +4721694436
Mob      +4792822774
Email:  kristian.jerpetjoen at gmail.com