[MPlayer-dev-eng] [PATCH] xvmc: handle shm xvimage allocation error
Reimar.Doeffinger at gmx.de
Mon Sep 12 18:26:27 CEST 2011
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;
> 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.
> + shmdt(Shminfo.shmaddr);
> + shmctl(Shminfo.shmid, IPC_RMID, 0);
> + 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.
More information about the MPlayer-dev-eng