[MPlayer-dev-eng] [PATCH] xvmc: handle shm xvimage allocation error
Marcin Slusarz
marcin.slusarz at gmail.com
Tue Sep 13 02:40:13 CEST 2011
Please Cc me on replies. I'm not subscribed.
On Mon, Sep 12, 2011 at 06:26:27PM +0200, =?ISO-8859-1?Q?Reimar_D=F6ffinger_ 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.
Ok. But it means we need to disable OSD as a fallback.
> > - 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.
It was a leftover from testing...
> 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.
Heh, localized shmat man page omitted this detail...
Fixed.
> > +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.
>
Half done. I think it's clearer the way I've done it, because shm_deallocate
could not share too much with deallocate_xvimage and need additional state
(did XShmAttach succeed?). If you feel stronly about it I can change this.
---
xvmc: handle osd xvimage allocation error
- handle xvimage/shm allocation failure and fallback to non-shm xv
- if non-shm xv still fails - disable OSD
- detect bug in xvmc implementation (xvimage->data_size==0) and disable OSD
Index: libvo/vo_xvmc.c
===================================================================
--- libvo/vo_xvmc.c (wersja 34098)
+++ libvo/vo_xvmc.c (kopia robocza)
@@ -148,7 +148,6 @@
//shm stuff from vo_xv
#ifdef HAVE_SHM
static XShmSegmentInfo Shminfo;
-static int Shmem_Flag;
#endif
static XvImage * xvimage;
@@ -181,48 +180,105 @@
return cur_render->state || (cur_render->mpi && cur_render->mpi->usage_count);
}
-static void allocate_xvimage(int xvimage_width,int xvimage_height,int xv_format)
+static int allocate_xvimage_normal(int xvimage_width, int xvimage_height, int xv_format)
{
- /*
- * allocate XvImages. FIXME: no error checking, without
- * mit-shm this will bomb... trzing to fix ::atmos
- */
+ xvimage = (XvImage *) XvCreateImage(mDisplay, xv_port, xv_format, NULL,
+ xvimage_width, xvimage_height);
+ if (!xvimage->data_size)
+ {
+ mp_msg(MSGT_VO, MSGL_WARN,
+ "XServer's XvCreateImage implementation is buggy (returned 0-sized image)\n" );
+ XFree(xvimage);
+ xvimage = NULL;
+ return -1;
+ }
+ xvimage->data = malloc(xvimage->data_size);
+ XSync(mDisplay,False);
+
+ return 0;
+}
+
#ifdef HAVE_SHM
- if ( mLocalDisplay && XShmQueryExtension( mDisplay ) ) Shmem_Flag = 1;
- else
+
+static int allocate_xvimage(int xvimage_width,int xvimage_height,int xv_format)
+{
+ int use_shm = mLocalDisplay && XShmQueryExtension( mDisplay );
+
+ Shminfo.shmid = -1;
+ Shminfo.shmaddr = (void *)-1;
+
+ if (!use_shm)
{
- Shmem_Flag = 0;
- mp_msg(MSGT_VO,MSGL_WARN, "Shared memory not supported\nReverting to normal Xv\n" );
+ mp_msg(MSGT_VO, MSGL_WARN, "Shared memory not supported\nReverting to normal Xv\n" );
+ return allocate_xvimage_normal(xvimage_width, xvimage_height, xv_format);
}
- if ( Shmem_Flag )
+
+ xvimage = (XvImage *) XvShmCreateImage(mDisplay, xv_port, xv_format,
+ NULL, xvimage_width, xvimage_height, &Shminfo);
+ if (!xvimage)
+ goto shmfail;
+
+ if (!xvimage->data_size)
{
- xvimage = (XvImage *) XvShmCreateImage(mDisplay, xv_port, xv_format,
- NULL, xvimage_width, xvimage_height, &Shminfo);
+ mp_msg(MSGT_VO, MSGL_WARN,
+ "XServer's XvShmCreateImage implementation is buggy (returned 0-sized image)\n" );
+ goto shmfail;
+ }
- Shminfo.shmid = shmget(IPC_PRIVATE, xvimage->data_size, IPC_CREAT | 0777);
- Shminfo.shmaddr = (char *) shmat(Shminfo.shmid, 0, 0);
- Shminfo.readOnly = False;
+ Shminfo.shmid = shmget(IPC_PRIVATE, xvimage->data_size, IPC_CREAT | 0777);
+ if (Shminfo.shmid == -1)
+ goto shmfail;
- xvimage->data = Shminfo.shmaddr;
- XShmAttach(mDisplay, &Shminfo);
- XSync(mDisplay, False);
+ Shminfo.shmaddr = (char *) shmat(Shminfo.shmid, 0, 0);
+ if (Shminfo.shmaddr == (void *)-1)
+ goto shmfail;
+
+ Shminfo.readOnly = False;
+
+ xvimage->data = Shminfo.shmaddr;
+ if (!XShmAttach(mDisplay, &Shminfo))
+ goto shmfail;
+
+ XSync(mDisplay, False);
+ shmctl(Shminfo.shmid, IPC_RMID, 0);
+
+ return 0;
+
+shmfail:
+ mp_msg(MSGT_VO, MSGL_WARN, "Failure in setting up shared memory Xv\nReverting to normal Xv\n" );
+ if (Shminfo.shmaddr != (void *)-1)
+ {
+ shmdt(Shminfo.shmaddr);
+ Shminfo.shmaddr = (void *)-1;
+ }
+ if (Shminfo.shmid != -1)
+ {
shmctl(Shminfo.shmid, IPC_RMID, 0);
+ Shminfo.shmid = -1;
}
- else
-#endif
+ if (xvimage)
{
- xvimage = (XvImage *) XvCreateImage(mDisplay, xv_port, xv_format, NULL, xvimage_width, xvimage_height);
- xvimage->data = malloc(xvimage->data_size);
- XSync(mDisplay,False);
+ XFree(xvimage);
+ xvimage = NULL;
}
-// memset(xvimage->data,128,xvimage->data_size);
- return;
+
+ return allocate_xvimage_normal(xvimage_width, xvimage_height, xv_format);
}
+#else
+
+static void allocate_xvimage(int xvimage_width,int xvimage_height,int xv_format)
+{
+ return allocate_xvimage_normal(xvimage_width, xvimage_height, xv_format);
+}
+
+#endif
+
+
static void deallocate_xvimage(void)
{
#ifdef HAVE_SHM
- if ( Shmem_Flag )
+ if (Shminfo.shmaddr != (void *)-1)
{
XShmDetach( mDisplay,&Shminfo );
shmdt( Shminfo.shmaddr );
@@ -812,7 +868,13 @@
mp_msg(MSGT_VO,MSGL_DBG4,"vo_xvmc: clearing subpicture\n");
clear_osd_fnc(0, 0, subpicture.width, subpicture.height);
- allocate_xvimage(subpicture.width, subpicture.height, subpicture_info.id);
+ if (allocate_xvimage(subpicture.width, subpicture.height, subpicture_info.id))
+ {
+ subpicture_mode = NO_SUBPICTURE;
+ mp_msg(MSGT_VO,MSGL_WARN, "vo_xvmc: OSD disabled\n");
+ return;
+ }
+
subpicture_alloc = 1;
}
More information about the MPlayer-dev-eng
mailing list