[MPlayer-dev-eng] Mplayer -vo ps3

Kristian Jerpetjøn kristian.jerpetjoen at gmail.com
Sat Apr 4 22:26:33 CEST 2009


General note i did not write most of the vo so im struggeling to
understand some of the things my self

2009/4/3 Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> On Fri, Apr 03, 2009 at 10:03:38PM +0200, Kristian Jerpetjøn wrote:
>> @@ -553,6 +553,7 @@
>>  SRCS_MPLAYER-$(DXR3)         += libvo/vo_dxr3.c
>>  SRCS_MPLAYER-$(ESD)          += libao2/ao_esd.c
>>  SRCS_MPLAYER-$(FBDEV)        += libvo/vo_fbdev.c libvo/vo_fbdev2.c
>> +SRCS_MPLAYER-$(PS3)       += libvo/vo_ps3.c
>
> Somehow wrong indentation.
sorted
Spaces spaces Everywhere!

>
>> +* Distributed AS IS.  Use at your own risk.  Author is not liable
>> +* in any way for any use, misuse, nonuse, expectation, failed
>> +* expectation, problem, or anything else.
>
> Is there a proper license somewhere? If that's all there is it would be
> unredistributable I think...
> Also in the end it should be using one of the default license headers,
> including the "GPLv2 or later" part, assuming all authors agree on it...

Changed to same license as vo_fbdev confirmed with co author

>
>> +* Changelog:
>> +*       12/22/2007 kristian jerpetj?n
>> +*           no longer double buffer frame buffer to accomodate 2.6.24
>> +*           now setting console graphics mode & text on exit (root only) - screen blanking issue
>> +*           begining removal of libps3fb dependency
>> +*      04/03/2009 kristian jerpetj?n
>> +*          removed dependency to libps3fb
>> +*          removed ebugging code
>> +*          fixed indentation
>> +*          removed dead code
>
> I don't think this is really useful...

removed

>
>> +#ifdef __powerpc__
>> +#include <malloc.h>
>> +#endif
>
> HAVE_MALLOC_H is the one I meant

now does
#include "config.h"

#if HAVE_MALLOC_H
#include<malloc.h>
#endif

>
>> +static void yuvcsc_run();
>> +static void yuvcsc_check();
>> +static void fix_scale ( void );
>> +static void snapshot();
>> +static int init_framebuffer();
>> +static void init_spu_medialib();
>> +static void update_spu_medialib();
>> +static void cleanup_spu_medialib();
>> +static void setup_scale();
>> +static void setup_screen();
>> +static int toggle_fullscreen();
>> +static void clear();
>> +static void draw_alpha ( int x0, int y0, int w, int h, unsigned char* src,
>> +unsigned char *srca, int stride );
>> +static void mpi_info ( mp_image_t * mpi );
>> +static void buf_info();
>
> You should just reorder the code so you don't need these.

removed

>
>> +static int yuvsW_req=16, yuvsH_req=4; //yuvscaler w&h division requirements

removed

>
> Use doxygen-compatible comments where possible.
>
>> +/* 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;
}


>
>> +static int preinit ( const char *arg )
>
> Diego will probably request that you use K&R style, which puts spaces like this:
>> static int preinit(const char *arg)

that will take some time.. working on it

>
>> +{
>> +
>> +opt_t subopts[] =    //See subopt_helper.h
>> +    {
>
> Should be const, indentation is weird.

fixed

>
>> +        { "debug",    OPT_ARG_BOOL,  &debug,     NULL},
>> +        { "noadj",    OPT_ARG_BOOL,  &noadj,     NULL},
>> +        { "snapshot",  OPT_ARG_BOOL,  &snap,  NULL},
>> +        { NULL,   0,  NULL,   NULL}
>
> Should be aligned nicely.

fixed

>
>> +    if ( subopt_parse ( arg, subopts ) )
>> +    {
>> +        mp_msg ( MSGT_VO, MSGL_WARN,
>> +                 "Suboptions for -vo ps3:\n"
>> +                 "  debug    - turn on debugging output specific to vo_ps3\n"
>> +                 "  noadj    - don't honor & scale to mplayer's suggested d_width x d_height\n"
>> +                 "  snapshot - take a raw yuv snapshot of input buffers on pause (to inbuf0/1.yuv)\n"
>> +                 "\n" );
>> +    }
>
> You shouldn't just continue if option parsing failed.

added
return -1;

>
>> +/* query_format - determine which formats are supported by this vo
>> + *
>> + * formats specifies image format - see libmpcodec/img_format.h
>> + */
>
> It's a standard VO API function, if you think it is badly documented help
> improve DOCS/tech/libvo.txt instead.

Definetly not badly documented probably copy pasted around
removed ..

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

>
>> +static int config ( uint32_t width, uint32_t height, uint32_t d_width, uint32_t d_height, uint32_t flags, char *title, uint32_t format )
>> +{
>> +
>> +    int i;
>> +
>> +    if ( vo_config_count > 0 ) //already config'd
>> +    {
>> +        return 0;
>> +    }
>
> That's wrong, it will break with -fixed-vo, you will probably end
> up producing a buffer overflow e.g. with
> mplayer -vo ps3 -fixed-vo small_resolution.avi large_resolution.avi
> at least it won't work right

removed check

>
>> +    src_aspect = ( float ) srcW / ( float ) srcH;
>
> Please find a way to use only the aspect.h code, if necessary extending it.

now uses aspect

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

>
>> +    if ( ( mpi->flags & MP_IMGFLAG_READABLE ) && ( mpi->type == MP_IMGTYPE_IPB || mpi->type == MP_IMGTYPE_IP ) )
>> +    {
>> +        if ( debug )
>> +        {
>> +            mp_msg ( MSGT_VO, MSGL_INFO, "[vo_ps3] get_image: READABLE -> stash in parking lot\n" );
>> +            mpi_info ( mpi );
>> +            buf_info();
>> +        }
>> +        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_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;
>> +    }
>
> That's completely wrong.
> First, you're not supposed to set MP_IMGFLAG_ALLOCATED yourself.
> Second, you should always set MP_IMGFLAG_DIRECT - yes, it is not draw
> directly onto the screen, but it is drawn directly into the VO/VF buffer,
> that's what that flag means.
> Lastly, MP_IMGTYPE_IP and MP_IMGTYPE_IPB are not the same, for IPB you need
> to reserve and additional frame, see vo_xv.
> Was this ever tested with any MPEG2 files? It should be more than obvious
> and result in completely corrupted output.

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

>
>> +    mpi->chroma_width = srcW/2;
>> +    mpi->chroma_height = srcH/2;
>
> Only mess with planes, strides, flags and priv, do _not_ touch anything else.

not touching this now

>
>> +    if ( !(mpi->flags & MP_IMGFLAG_DIRECT) )
>> +    {
>
> Use
> if (mpi->flags & MP_IMGFLAG_DIRECT)
>    return 0;
> to reduce the indentation level and complexity.
>
>> +        //todo: not (mpi->flags & MP_IMGFLAG_DIRECT) (no -dr flag for direct rendering),
>> +        // may need to handle MP_IMGFLAG_READABLE IPB (B) frames differently
>> +        // When direct rendering is enabled (-dr), I moved the B frames to a non-direct
>> +        // buffer and then when drawn with draw_image, they (seem) to work fine
>> +        // BUT, without direct rendering, don't know how to handle them :(
>> +        // Just drawing them as called makes a mess of the video.
>> +        if ( ( mpi->flags & MP_IMGFLAG_READABLE ) &&
>> +                ( mpi->type == MP_IMGTYPE_IPB || mpi->type == MP_IMGTYPE_IP ) )
>
> You're not supposed to care about any of these here, _no_ other vo does that, just
> directly displaying whatever they get works perfectly for them.
> Note that if you so far have extra buffers for I, P, B and you need to use a buffer here,
> too, you need to use a different one.
> Even with directrendering enabled, this function might be called with IMGTYPE_TEMP or whatever.

Same comment as above i need serious help correcting this.

>
>> +            if ( mpi->stride[i] == src_stride[i] )
>> +            {
>> +                fast_memcpy ( buf_plane_ptr[i], mpi->planes[i], mpi_p_siz[i] );
>> +            }
>> +            else
>> +            {
>> +
>> +                memcpy_pic ( buf_plane_ptr[i], mpi->planes[i], mpi_p_w[i], mpi_p_h[i], src_stride[i], mpi->stride[i] );
>> +            }
>
> That is silly, memcpy_pic already does exactly that optimization.

Corrected now only does a memcpy_pic

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

>
>> +static uint32_t start_slice ( mp_image_t *mpi )
>> +{
>> +    // do nothing
>> +    return 0;
>> +}
>
> If you don't need it, remove it.

removed

>
>> +    //todo: should we care about MP_IMGFLAG_PRESERVE for a slice?
>
> Huh? Is the input buffer modified?

dont think so

>
>> +        //planes y,u,v
>> +        if ( stride[i] == src_stride[i] )
>> +        {
>> +            fast_memcpy ( buf_plane_ptr[i]+offset[i], src[i], slice_siz[i] );
>> +        }
>> +        else
>> +        {
>> +
>> +            memcpy_pic ( buf_plane_ptr[i]+offset[i], src[i], slice_p_w[i], slice_p_h[i], src_stride[i], stride[i] );
>> +        }
>
> as above...

fixed


>
>> +            return query_format ( * ( ( uint32_t* ) data ) );
>
> One set of () too much.

fixed

>
>> +        case VOCTRL_RESET:             /* 3 */
>> +            if ( debug )
>> +                mp_msg ( MSGT_VO, MSGL_INFO, "[vo_ps3] control: todo: handle RESET (%u)\n", request );
>> +            return VO_NOTIMPL;
>
> I doubt you have anything to handle here, just get rid of this.
> And get rid of those number comments, who cares which number is behind
> those VOCTRL?

fixed

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

>
>> +    if (console)
>> +        close ( console );
>> +    if (fb)
>> +        close ( fb );
>
> Bad idea, 0 is a valid fd and -1 is an invalid fd.

fixed

if (console >= 0)
 close(console)
etc

>
>> +/* yuvcsc_run - time to put the image on the back framebuffer
>> + *
>> + *
>> + */
>
> Doxygen

Working on it ;)

>
>> +static void yuvcsc_run()
>
> missing void

fixed

>
>> +/* yuvcsc_check - check spu_yuv2argb_scaler process
>> + *
>> + * make sure it's ready to run, and if not wait
>> + */
>> +static void yuvcsc_check()

fixed

>
> as above
>
>> +    if ( wait_yuvcsc )                                 //make sure last run finished
>> +    {
>
> if (!wait_yuvcsc)
>    return;
> makes it more obvious to understand.

done

>
>> +    //todo:  MAJOR BUG BELOW! Normally, this should be fine, but if mplayer is called
>> +    //       with -vf dsize=someX:someY and -vo ps3:noadj, and that someX or someY is way
>> +    //       out of scale, then the calc above might create a dstH that's way too big
>> +    //       and overrun the framebuffer.
>
> Well, those functions in aspect.h are there so that we don't get everyone
> reimplementing them wrong.

belive this is now sorted

>
>> +/* snapshot - copy inbufs to .yuv files in current directory
>> + *
>> + * handy for testing - try with single stepping! '.'
>> + */
>> +static void snapshot()
>
> doxy, void as above.

done

> Well, once the bugs are cleaned up (i.e. before it is committed) this should be removed anyway.

agree


>
>> +static int toggle_fullscreen ( void )
>
> Why int?

now void

>
>> +    vo_fs^=1;
>
> Hm.
> vo_fs = !vo_fs;
> is more reliable.

changed

>
>> +static void setup_scale()
>
> missing void

fixed

>
>> +{
>> +    if ( noadj )   //asked not to honor mplayer's size
>
> -noaspect does that, don't make people relearn MPlayer by doing everything
> a bit differently.

removed noadj

>
>> +    if ( ( dstW > vo_screenwidth ) || ( dstH > vo_screenheight ) || vo_fs )
>
> a few () too many, anyway aspect.h code probably actually works right...

changed

>
>> +static void setup_screen()
>
> missing void

fixed

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

>
>> +static int init_framebuffer()
>
> Missing void.

fixed

>
>> +    //ok the previous failed lets attempt to open as root
>> +    if ( ( console < 0 ) || ( res < 0 ) )
>
> Too many ()

fixed

>
>> +    {
>> +        close ( console );
>
> Not such a great idea if console < 0

handled correctly

>
>> +    //all options to open the FB has failed.. So lets bail..
>> +    if ( ( res < 0 ) || ( console < 0 ) )
>
> () and inconsistent order, above you checked res last...

changed

>
>> +    if ( arg != KD_GRAPHICS )
>> +    {
>> +        if ( ioctl ( console,KDSETMODE,KD_GRAPHICS ) < 0 )
>> +        {
>> +
>> +            return -1;
>> +        }
>> +    }
>
> if (arg != KD_GRAPHICS && ioctl(...))
>    return -1;

changed

>
>> +    //ok we asume the console blanking is killed.. now its "safe" to handle the fb
>
> Uh, this was only for console blanking? I don't think it is reasonable at all
> to fail completely just because we could not disable console blanking.

now keeps going in case that failed

>
>> +    fb = open ( FB_DEV,O_RDWR );
>> +
>> +    //major problem if it occurs
>> +    if ( fb < 0 )
>> +    {
>> +        mp_msg ( MSGT_VO, MSGL_INFO, "[vo_ps3] init_framebuffer: failed to open fb\n" );
>> +        res=-1;
>> +    }
>> +    if ( ioctl ( fb, PS3FB_IOCTL_SCREENINFO, ( unsigned long ) &ps3_fb ) < 0 )
>> +    {
>> +        mp_msg ( MSGT_VO, MSGL_INFO, "[vo_ps3] init_framebuffer: failed to get fb info\n" );
>> +        res=-1;
>> +    }
>> +
>> +    if ( ioctl ( fb, PS3FB_IOCTL_ON ) < 0 )
>> +    {
>> +        mp_msg ( MSGT_VO, MSGL_INFO, "[vo_ps3] init_framebuffer: failed to set manual swap control\n" );
>> +        res=-1;
>> +    }
>> +
>> +    if ( res < 0 )
>> +    {
>> +        ioctl ( console,KDSETMODE,KD_TEXT );
>> +        close ( console );
>> +        ioctl ( fb,PS3FB_IOCTL_OFF );
>> +        close ( fb );
>> +        return -1;
>> +    }
>
> Calling ioctl on an invalid fd (if fb == -1) surely is not the greatest ide. Consider using goto...

now has a goto teardown

>
>> +    fb_aspect = ( float ) fbW / ( float ) fbH;
>
> Completely ignore -monitoraspect / -monitorpixelaspect

removed

>
>> +    mp_msg ( MSGT_VO, MSGL_INFO, "[vo_ps3] init_spu_medialib: Initialized spu-medialib's           spu_yuv2argb_scaler with:\n %ix%i=>%ix%i, offset:%i, maxW:%i\n", srcW, srcH, dstW, dstH, offset, maxW );
>
> Missing a linebreak or two, also this is at most MSGL_V, not MSGL_INFO, most
> users will _not_ care about this. Might apply to other places as well,
> I have not paid attention.

Have to do more on this but cleaned up the missing lines

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

>
>> +static void cleanup_spu_medialib()
>
> missing "void". I probably missed a few more of those cases...

fixed

>
>> +static void clear()
>
> void

fixed

>
>> +    mp_msg ( MSGT_VO, MSGL_INFO, "[vo_ps3] clear: Clearing buffers\n" );
>> +
>> +    for ( i=0; i<NUM_BUFFERS; i++ )
>> +    {
>> +        memset ( inbuf_y[i], 0, src_p_siz[0] );
>> +        memset ( inbuf_u[i], 0, src_p_siz[1] );
>> +        memset ( inbuf_v[i], 0, src_p_siz[2] );
>> +        memset ( parking_lot_y[i], 0, src_p_siz[0] );
>> +        memset ( parking_lot_u[i], 0, src_p_siz[1] );
>> +        memset ( parking_lot_v[i], 0, src_p_siz[2] );
>
> I sure can't see a point of clearing the buffers to an ugly green...
> Maybe you should just get rid of this?

removed

>
>> +static void mpi_info ( mp_image_t * mpi )
>> +static void buf_info()
>
> Missing void.
> And debug stuff -> should go before the final version (no hurry though).

removed these two for now

>
> Greetings,
> Reimar Döffinger
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
>

I am not done with all the comments but around 80% ish is taken care of.


Some notes on testing
without -dr causes tearing on some avi and mpeg files
with -dr seems fine

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

This can be changed to vary on every frame using the update interface
but i should perhaps rewrite the api to be oriented around a struct
instead at the same time.

However these buffers need to be aligned and allocated and to be left
alone while the other asynch processors(DSP's is a way too look at
them)
are working on them.

Please advise further actions

Cheers
-- 
Kristian Jerpetjøn
Tlf:        +4721694436
Mob      +4792822774
Email:  kristian.jerpetjoen at gmail.com
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: mplayer-vo_ps3-r3.txt
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090404/fd8da66a/attachment.txt>


More information about the MPlayer-dev-eng mailing list