[MPlayer-dev-eng] [PATCH] xvmc: handle shm xvimage allocation error

[Ivan Kalvachev]

On 9/12/11, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> On Sat, Sep 10, 2011 at 11:58:33PM +0200, Marcin Slusarz wrote:
>> - assert xvimage->data_size!=0 because 0 size will lead to memory
>> corruptions
>>   (I hit it because of bug in Nouveau's early xvmc implementation)
>
> An assert is not really that much better.
> Failing is fine. So is checking data_size at the appropriate place.
> But an assert isn't a good enough solution to be worth it IMO.
>
>> -    if ( mLocalDisplay && XShmQueryExtension( mDisplay ) ) Shmem_Flag =
>> 1;
>> +    if ( mLocalDisplay && XShmQueryExtension( mDisplay ) )
>> +        Shmem_Flag = 0;
>>      else
>>      {
>>          Shmem_Flag = 0;
>
> That is rather pointless if you set it to 0 in both cases.
> And the warning about not using Shm will no longer really
> correspond to reality after this change.
> Lastly, this change causes deallocate_xvimage to always use free
> instead of XShmDetach when it should.
>
>> @@ -198,25 +208,41 @@
>>      {
>>          xvimage = (XvImage *) XvShmCreateImage(mDisplay, xv_port,
>> xv_format,
>>                               NULL, xvimage_width, xvimage_height,
>> &Shminfo);
>> +        if (!xvimage)
>> +            goto noshmimage;
>>
>> +        assert(xvimage->data_size);
>>          Shminfo.shmid    = shmget(IPC_PRIVATE, xvimage->data_size,
>> IPC_CREAT | 0777);
>> +        if (Shminfo.shmid == -1)
>> +            goto shmgetfail;
>> +
>>          Shminfo.shmaddr  = (char *) shmat(Shminfo.shmid, 0, 0);
>> +        if ((long)Shminfo.shmaddr == -1)
>
> Documentation says it returns (void *)-1, so you should check for
> equality with that really.
>
>> +shmattachfail:
>> +    shmdt(Shminfo.shmaddr);
>> +shmatfail:
>> +    shmctl(Shminfo.shmid, IPC_RMID, 0);
>> +shmgetfail:
>> +    XFree(xvimage);
>
> This would be much nicer (i.e. require only a single goto)
> if initializing shmaddr/shmid/xvimage to appropriate invalid
> values, adding a shm_deallocate function that frees those
> that have a valid value and then reusing that in deallocate_xvimage
> should also get rid of the need for Shmem_Flag.

imho, I prefer the goto approach more.
It is indeed worse in language cleanness, but the generated code is
more optimal (not that it is speed critical:), because you won't check
for same thing twice.
I also find it more readable, (aka use the goto only in case of exact
error, otherwise it becomes mess with special values).


I'd just like to assert something. (not directly related to this patch).

Few months ago I investigated a user problem with Xv that turned out
to be caused by too small memory size for shm. So the xvimage
allocation is problem that have to be fixed in vo_xv.c too and we have
multiple xvimage buffers there.

So there are basically 2 solutions:
1. In case of Shm allocation fails, free all existing Shm images and
clear the flag, aka stop using Shm at all.
2. Make individual flag for each buffer and query that flag at each
use... IMHO this is overkill.

Moving the code to x11_common.c would be great, I had an attempt but I
didn't like it (i didn't like sharing the Shmem_Flag) , that's why I
haven't posted it .

P.S.
Good work on the patch, by the way.