[FFmpeg-devel] x11grab.c: minor clean up, added more documentation

[Sven C. Dack]

On 01/04/11 11:13, Carl Eugen Hoyos wrote:
> Sven C. Dack<sven.c.dack<at>  virginmedia.com>  writes:
>
>    
>> -    char *param, *offset;
>> +    char *dpyname, *offset;
>>      
> If the variable renaming is necessary, it should be in a separate patch.
>
>    
>> -    param = av_strdup(s1->filename);
>> -    offset = strchr(param, '+');
>> +    dpyname = av_strdup(s1->filename);
>> +    x11grab->dpyname = dpyname;
>> +
>> +    offset = strchr(dpyname, '+');
>>      
> ... making this diff significantly smaller.
>
>    
>>       if (offset) {
>>           sscanf(offset, "%d,%d",&x_off,&y_off);
>> -        x11grab->nomouse= strstr(offset, "nomouse");
>> -        *offset= 0;
>> +        x11grab->nomouse = strstr(offset, "+nomouse") == NULL ? 0 : 1;
>> +        *offset = '\0';
>>      
> If I understand it correctly, this change is definitely not appropriate.
>
>    
>>       }
>>
>> -    av_log(s1, AV_LOG_INFO, "device: %s ->  display: %s x: %d y: %d width: %d
>>      
> height: %d\n", s1->filename,
>    
>> param, x_off, y_off, ap->width, ap->height);
>> +    av_log(s1, AV_LOG_INFO, "device: %s ->  display: %s x: %d y: %d width: %d
>>      
> height: %d mouse: %s\n",
>    
>> s1->filename, dpyname, x_off, y_off, ap->width, ap->height, x11grab->nomouse ?
>>      
> "no" : "yes");
>
> Separate patch, please.
>
> [...]
>
>    
>>       use_shm = XShmQueryExtension(dpy);
>> -    av_log(s1, AV_LOG_INFO, "shared memory extension %s found\n", use_shm ?
>>      
> "" : "not");
>    
>> +    av_log(s1, AV_LOG_INFO, "shared memory extension%s found\n", use_shm ? ""
>>      
> : " not");
>
> This is an unrelated change and should go into a separate patch.
>
>    
>> +    av_free(x11grab->dpyname);
>>      
> So I guess my solution, to free param immediately after the call to
> XOpenDisplay(), was wrong;-)
>    
What are you talking about? As far as I can tell did you not free it at 
all. You named a variable simply "param" for parameter, which is dumb 
and I am guessing you already know this. The code which you then find 
inappropriate does not throw a warning message like yours did (->nomouse 
is an integer whereas strstr() returns a pointer) and you want me to 
break a patch, which is a small patch, into three patches because it is 
too big? Your email is bigger than the patch. Care to explain why 
exactly you send me this mail? If not then please keep your criticism to 
a minimum.

Sven Dack

> Carl Eugen
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>