[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