[MPlayer-dev-eng] Mplayer -vo ps3
Reimar Döffinger
Reimar.Doeffinger at gmx.de
Fri Apr 3 23:22:45 CEST 2009
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.
> +* 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...
> +* 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...
> +#ifdef __powerpc__
> +#include <malloc.h>
> +#endif
HAVE_MALLOC_H is the one I meant
> +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.
> +static int yuvsW_req=16, yuvsH_req=4; //yuvscaler w&h division requirements
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.
> +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)
> +{
> +
> +opt_t subopts[] = //See subopt_helper.h
> + {
Should be const, indentation is weird.
> + { "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.
> + 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.
> +/* 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.
> + 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...
> +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
> + src_aspect = ( float ) srcW / ( float ) srcH;
Please find a way to use only the aspect.h code, if necessary extending it.
> + vo_doublebuffering = VO_TRUE;
> + vo_directrendering = VO_TRUE;
These really should be left to the user and not overridden.
> + 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.
> + mpi->chroma_width = srcW/2;
> + mpi->chroma_height = srcH/2;
Only mess with planes, strides, flags and priv, do _not_ touch anything else.
> + 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.
> + 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.
> +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.
> +static uint32_t start_slice ( mp_image_t *mpi )
> +{
> + // do nothing
> + return 0;
> +}
If you don't need it, remove it.
> + //todo: should we care about MP_IMGFLAG_PRESERVE for a slice?
Huh? Is the input buffer modified?
> + //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...
> + return query_format ( * ( ( uint32_t* ) data ) );
One set of () too much.
> + 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?
> + 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).
> + if (console)
> + close ( console );
> + if (fb)
> + close ( fb );
Bad idea, 0 is a valid fd and -1 is an invalid fd.
> +/* yuvcsc_run - time to put the image on the back framebuffer
> + *
> + *
> + */
Doxygen
> +static void yuvcsc_run()
missing void
> +/* yuvcsc_check - check spu_yuv2argb_scaler process
> + *
> + * make sure it's ready to run, and if not wait
> + */
> +static void yuvcsc_check()
as above
> + if ( wait_yuvcsc ) //make sure last run finished
> + {
if (!wait_yuvcsc)
return;
makes it more obvious to understand.
> + //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.
> +/* snapshot - copy inbufs to .yuv files in current directory
> + *
> + * handy for testing - try with single stepping! '.'
> + */
> +static void snapshot()
doxy, void as above.
Well, once the bugs are cleaned up (i.e. before it is committed) this should be removed anyway.
> +static int toggle_fullscreen ( void )
Why int?
> + vo_fs^=1;
Hm.
vo_fs = !vo_fs;
is more reliable.
> +static void setup_scale()
missing void
> +{
> + if ( noadj ) //asked not to honor mplayer's size
-noaspect does that, don't make people relearn MPlayer by doing everything
a bit differently.
> + if ( ( dstW > vo_screenwidth ) || ( dstH > vo_screenheight ) || vo_fs )
a few () too many, anyway aspect.h code probably actually works right...
> +static void setup_screen()
missing void
> + 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...
> +static int init_framebuffer()
Missing void.
> + //ok the previous failed lets attempt to open as root
> + if ( ( console < 0 ) || ( res < 0 ) )
Too many ()
> + {
> + close ( console );
Not such a great idea if console < 0
> + //all options to open the FB has failed.. So lets bail..
> + if ( ( res < 0 ) || ( console < 0 ) )
() and inconsistent order, above you checked res last...
> + if ( arg != KD_GRAPHICS )
> + {
> + if ( ioctl ( console,KDSETMODE,KD_GRAPHICS ) < 0 )
> + {
> +
> + return -1;
> + }
> + }
if (arg != KD_GRAPHICS && ioctl(...))
return -1;
> + //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.
> + 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...
> + fb_aspect = ( float ) fbW / ( float ) fbH;
Completely ignore -monitoraspect / -monitorpixelaspect
> + 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.
> + 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?!?
> +static void cleanup_spu_medialib()
missing "void". I probably missed a few more of those cases...
> +static void clear()
void
> + 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?
> +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).
Greetings,
Reimar Döffinger
More information about the MPlayer-dev-eng
mailing list