[vlc-devel] [PATCH] vlc: minimal_macosx fix
Felix Paul Kühne
fkuehne.videolan at gmail.com
Sun Oct 7 16:06:06 CEST 2012
Hello,
thanks for your work!
For such a big commit, could you please include more comments within the commit message, so future developers don't have to dive into the vlc-devel archives to see what's going on?
More comments in line..
On Oct 6, 2012, at 3:18 PM, James Bates wrote:
> diff --git a/modules/gui/minimal_macosx/Modules.am b/modules/gui/minimal_macosx/Modules.am
> index cf22105..cf3b0ec 100644
> --- a/modules/gui/minimal_macosx/Modules.am
> +++ b/modules/gui/minimal_macosx/Modules.am
> @@ -9,12 +9,10 @@ SOURCES_minimal_macosx = \
> VLCMinimalVoutWindow.m \
> VLCOpenGLVoutView.m \
> voutgl.m \
> - voutagl.m \
> $(NULL)
>
> noinst_HEADERS = \
> intf.h \
> VLCMinimalVoutWindow.h \
> VLCOpenGLVoutView.h \
> - voutagl.h \
> voutgl.h
The voutagl stuff won't be fixed since it is deprecated technology. So, if you are removing it from the build system please also remove the actual files.
> diff --git a/modules/gui/minimal_macosx/VLCOpenGLVoutView.m b/modules/gui/minimal_macosx/VLCOpenGLVoutView.m
> index 4fa69c1..83c969d 100644
> --- a/modules/gui/minimal_macosx/VLCOpenGLVoutView.m
> +++ b/modules/gui/minimal_macosx/VLCOpenGLVoutView.m
> @@ -39,80 +39,80 @@
> #include <OpenGL/OpenGL.h>
> #include <OpenGL/gl.h>
>
> +
> /*****************************************************************************
> * cocoaglvoutviewInit
> *****************************************************************************/
> -int cocoaglvoutviewInit( vout_thread_t * p_vout )
> +int cocoaglvoutviewInit( vout_window_t *p_wnd, const vout_window_cfg_t *cfg)
> {
> vlc_value_t value_drawable;
> id <VLCOpenGLVoutEmbedding> o_cocoaglview_container;
>
> - msg_Dbg( p_vout, "Mac OS X Vout is opening" );
> + msg_Dbg( p_wnd, "Mac OS X Vout is opening" );
>
> - var_Create( p_vout, "drawable-nsobject", VLC_VAR_DOINHERIT );
> - var_Get( p_vout, "drawable-nsobject", &value_drawable );
> + var_Create( p_wnd, "drawable-nsobject", VLC_VAR_DOINHERIT );
> + var_Get( p_wnd, "drawable-nsobject", &value_drawable );
>
> - p_vout->p_sys->o_pool = [[NSAutoreleasePool alloc] init];
> + p_wnd->sys->o_pool = [[NSAutoreleasePool alloc] init];
>
> /* This will be released in cocoaglvoutviewEnd(), on
> * main thread, after we are done using it. */
> o_cocoaglview_container = [(id) value_drawable.p_address retain];
> if (!o_cocoaglview_container)
> {
> - msg_Warn( p_vout, "No drawable!, spawing a window" );
> + msg_Warn( p_wnd, "No drawable!, spawing a window" );
> }
>
> - p_vout->p_sys->b_embedded = false;
> + //p_vout->p_sys->b_embedded = false;
>
>
> /* Create the GL view */
> - struct args { vout_thread_t * p_vout; id <VLCOpenGLVoutEmbedding> container; } args = { p_vout, o_cocoaglview_container };
> + struct args { vout_window_t *p_wnd; vout_window_cfg_t *cfg; id <VLCOpenGLVoutEmbedding> container; } args = { p_wnd, cfg, o_cocoaglview_container };
This leads to the following compilation warning:
VLCOpenGLVoutView.m:70:122: warning: initializing 'vout_window_cfg_t *' with an expression of type 'const vout_window_cfg_t *' discards qualifiers [-Wincompatible-pointer-types]
...container; } args = { p_wnd, cfg, o_cocoaglview_container };
> /*****************************************************************************
> * cocoaglvoutviewManage
> *****************************************************************************/
> -int cocoaglvoutviewManage( vout_thread_t * p_vout )
> +/*int cocoaglvoutviewManage( vout_thread_t * p_vout )
> {
> if( p_vout->i_changes & VOUT_ASPECT_CHANGE )
> {
> @@ -144,11 +144,11 @@ int cocoaglvoutviewManage( vout_thread_t * p_vout )
> //[[p_vout->p_sys->o_glview container] manage];
> return VLC_SUCCESS;
> }
> -
> +*/
> /*****************************************************************************
> * cocoaglvoutviewControl: control facility for the vout
> *****************************************************************************/
> -int cocoaglvoutviewControl( vout_thread_t *p_vout, int i_query, va_list args )
> +/*int cocoaglvoutviewControl( vout_thread_t *p_vout, int i_query, va_list args )
> {
> bool b_arg;
>
> @@ -163,20 +163,20 @@ int cocoaglvoutviewControl( vout_thread_t *p_vout, int i_query, va_list args )
> return VLC_EGENERIC;
> }
> }
> -
> +*/
> /*****************************************************************************
> * cocoaglvoutviewSwap
> *****************************************************************************/
> -void cocoaglvoutviewSwap( vout_thread_t * p_vout )
> +/*void cocoaglvoutviewSwap( vout_thread_t * p_vout )
> {
> p_vout->p_sys->b_got_frame = true;
> [[p_vout->p_sys->o_glview openGLContext] flushBuffer];
> }
> -
> +*/
> /*****************************************************************************
> * cocoaglvoutviewLock
> *****************************************************************************/
> -int cocoaglvoutviewLock( vout_thread_t * p_vout )
> +/*int cocoaglvoutviewLock( vout_thread_t * p_vout )
> {
> if( kCGLNoError == CGLLockContext([[p_vout->p_sys->o_glview openGLContext] CGLContextObj]) )
> {
> @@ -185,15 +185,15 @@ int cocoaglvoutviewLock( vout_thread_t * p_vout )
> }
> return 1;
> }
> -
> +*/
> /*****************************************************************************
> * cocoaglvoutviewUnlock
> *****************************************************************************/
> -void cocoaglvoutviewUnlock( vout_thread_t * p_vout )
> +/*void cocoaglvoutviewUnlock( vout_thread_t * p_vout )
> {
> CGLUnlockContext([[p_vout->p_sys->o_glview openGLContext] CGLContextObj]);
> }
> -
> +*/
Please don't comment code you don't intend to maintain. Just remove it completely. If some weirdo wants to reimplement code targeting 10.4 and earlier, the code is still there in git.
> /*****************************************************************************
> * VLCOpenGLVoutView implementation
> *****************************************************************************/
> @@ -203,22 +203,22 @@ void cocoaglvoutviewUnlock( vout_thread_t * p_vout )
> * vout_thread_t. Must be called from main thread. */
> + (void) autoinitOpenGLVoutViewIntVoutWithContainer: (NSData *) argsAsData
> {
> - NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
> - struct args { vout_thread_t * p_vout; id <VLCOpenGLVoutEmbedding> container; } *
> + //NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
> + struct args { vout_window_t *p_wnd; vout_window_cfg_t *cfg; id <VLCOpenGLVoutEmbedding> container; } *
> args = (struct args *)[argsAsData bytes];
> VLCOpenGLVoutView * oglview;
>
> if( !args->container )
> {
> - args->container = [[VLCMinimalVoutWindow alloc] initWithContentRect: NSMakeRect( 0, 0, args->p_vout->i_window_width, args->p_vout->i_window_height )];
> + args->container = [[VLCMinimalVoutWindow alloc] initWithContentRect: NSMakeRect( 0, 0, args->cfg->width, args->cfg->height )];
> [(VLCMinimalVoutWindow *)args->container makeKeyAndOrderFront: nil];
> }
> - oglview = [[VLCOpenGLVoutView alloc] initWithVout: args->p_vout container: args->container];
> + oglview = [[VLCOpenGLVoutView alloc] initWithVoutWindow: args->p_wnd container: args->container];
>
> - args->p_vout->p_sys->o_glview = oglview;
> + args->p_wnd->handle.nsobject = oglview;
> [args->container addVoutSubview: oglview];
>
> - [pool release];
> + //[pool release];
> }
It's true that this method doesn't need an auto release pool if its own. Why not remove it?
> diff --git a/modules/gui/minimal_macosx/intf.m b/modules/gui/minimal_macosx/intf.m
> index b9a13ef..6def200 100644
> --- a/modules/gui/minimal_macosx/intf.m
> +++ b/modules/gui/minimal_macosx/intf.m
> @@ -54,6 +54,7 @@ static void Run ( intf_thread_t *p_intf );
> int OpenIntf ( vlc_object_t *p_this )
> {
> intf_thread_t *p_intf = (intf_thread_t*) p_this;
> + msg_Dbg( p_intf, "## Opening the Minimal Mac OS X module" );
Huh? Why the double hashes here and in more messages below?
> p_intf->p_sys = malloc( sizeof( intf_sys_t ) );
> if( p_intf->p_sys == NULL )
> @@ -64,7 +65,7 @@ int OpenIntf ( vlc_object_t *p_this )
> memset( p_intf->p_sys, 0, sizeof( *p_intf->p_sys ) );
>
> p_intf->pf_run = Run;
> - p_intf->b_should_run_on_first_thread = true;
> +// p_intf->b_should_run_on_first_thread = true;
>
> return VLC_SUCCESS;
> }
Please just remove this deprecated API call.
> @@ -110,7 +111,7 @@ static void * KillerThread( void *user_data )
> vlc_mutex_destroy( &p_intf->p_sys->lock );
> vlc_cond_destroy( &p_intf->p_sys->wait );
>
> - msg_Dbg( p_intf, "Killing the Minimal Mac OS X module" );
> + msg_Dbg( p_intf, "## Killing the Minimal Mac OS X module" );
>
> /* We are dead, terminate */
> [NSApp terminate: nil];
> @@ -123,6 +124,8 @@ static void * KillerThread( void *user_data )
> *****************************************************************************/
> static void Run( intf_thread_t *p_intf )
> {
> + msg_Dbg( p_intf, "## Running the Minimal Mac OS X module" );
> +
> sigset_t set;
>
> /* Make sure the "force quit" menu item does quit instantly.
>
> diff --git a/modules/gui/minimal_macosx/voutgl.h b/modules/gui/minimal_macosx/voutgl.h
> index 3e51ea0..edac8a5 100644
> --- a/modules/gui/minimal_macosx/voutgl.h
> +++ b/modules/gui/minimal_macosx/voutgl.h
> @@ -48,3 +48,10 @@ struct vout_sys_t
> bool b_clipped_out;
> Rect clipBounds, viewBounds;
> };
> +
> +struct vout_window_sys_t
> +{
> + NSAutoreleasePool *o_pool;
> +};
Looks weird and unused to me. Above this structure are lots of declarations, which seem to be unused, too. Let's remove them, no?
The rest of your patch looks good!
Cheers,
Felix
More information about the vlc-devel
mailing list