[vlc-devel] [PATCH] vlc: minimal_macosx fix

[James Bates]

> Date: Sun, 7 Oct 2012 16:06:06 +0200
> From: Felix Paul K?hne <fkuehne.videolan at gmail.com>
> To: Mailing list for VLC media player developers
> 	<vlc-devel at videolan.org>
> Subject: Re: [vlc-devel] [PATCH] vlc: minimal_macosx fix
> Message-ID: <30C89D82-FC45-4C15-8A9B-1D803D24140C at gmail.com>
> Content-Type: text/plain; charset=us-ascii
> 
> 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?
OK, that basically means the subject line of the message right? I will describe it more carefully in my resubmission of the patch.

> 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.
I've removed them.

>> 
>>    /* 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 };
Fixed.

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

> 
>> /*****************************************************************************
>> * 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?
Done.

> 
>> 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?
That was just me trying to figure out the call-sequences. I've removed the debug messages completely.


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

>> 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?
It is used in VLCOpenGLVoutView.m, lines 56 and 107.

I have resubmitted the corrected patch in a new message. Thanks for the review!

James