Re: [Mplayerplug-in-devel] Patches
Brought to you by:
kdekorte
From: David M. <dme...@be...> - 2004-01-31 12:57:59
|
On Thu, Jan 29, 2004 at 12:00:02AM -0200, Alexandre Pereira da Silva wrote: > My patches in one unified diff. I made some changes as sugested by David. > I need to find another way to get the proxy settings, as that code is not ideal. I have not included proxy support on this patch. > > Index: Source/mplayerplug-in.c > =================================================================== > RCS file: /cvsroot/mplayerplug-in/mplayerplug-in/Source/mplayerplug-in.c,v > retrieving revision 1.150 > diff -u -r1.150 mplayerplug-in.c > --- Source/mplayerplug-in.c 26 Jan 2004 17:16:34 -0000 1.150 > +++ Source/mplayerplug-in.c 29 Jan 2004 01:52:52 -0000 > @@ -287,6 +287,9 @@ > if (instance == NULL) > return NPERR_INVALID_INSTANCE_ERROR; > > + //Get rid of zombie processes > + signal(SIGCHLD, SIG_IGN); This won't work on all of the time (particularly, it won't on Linux < 2.6 (it may work on Redhat's 2.4 because they patch their kernel extensively)). On 2.4, this has no effect I believe. However, this is not guaranteed by POSIX to work. That said, I haven't actually seen any zombie processes on my 2.4 system. This is probably because the mplayer process is created from another thread (which is actually treated as a separate process on 2.4 without CLONE_THREAD), and when that thread exits, mplayer gets reparented to init, which calls waitpid(). So, zombies happen to not be a problem on 2.4. But, you may see them on 2.6 or with Fedora. Has anyone seen them there without this change? In any case, since this is extermely non-portable, and also, interferes with other processes that the browser or other plugins want to start and call waitpid() on, I don't think this is a good idea. The right way to do this would be to call waitpid() from the playPlaylist thread (_without_ WNOHANG) around the same time the control stream is closed, just like pclose() would. > instance->pdata = NPN_MemAlloc(sizeof(PluginInstance)); > This = (PluginInstance *) instance->pdata; > InitPrivateData(instance); > @@ -402,8 +405,7 @@ > > NPError NPP_Destroy(NPP instance, NPSavedData ** save) > { > - > - char cmd[1024]; > + int count, status; > PluginInstance *This; > int ret; > > @@ -425,6 +427,26 @@ > > if (DEBUG) > ret = fclose(This->logfp); > + > + if(!This->height && !This->width && This->pid) { > + if(DEBUG) > + printf("Trying to kill mplayer process(%d), if it still exists\n", This->pid); > + > + sendCommand(This->control, "quit"); > + > + count = 0; > + status = 1; > + while(status && count<10) { > + status = kill(This->pid, 15); > + count++; > + } Doesn't This->pid need locking here, because it's set in another thread? > void *playPlaylist(void *td) > { > > @@ -1158,6 +1191,8 @@ > char message[1024]; > char mmsplaylist[1024]; > char *eos; > + char *argv[256]; > + int argc=0; > ThreadData *local_td; > Node *local_list, *copy; > int i; > @@ -1198,11 +1233,11 @@ > i = 0; > if (DEBUG) > printf("building command string\n"); > + > while (strcmp(local_td->argv[i], "") != 0) { > + argv[argc++]=local_td->argv[i]; > if (DEBUG) > printf("PLAY %i:%s\n", i, local_td->argv[i]); > - strlcat(cmd, local_td->argv[i], sizeof(cmd)); > - strlcat(cmd, " ", sizeof(cmd)); > i++; > } > > @@ -1237,19 +1272,17 @@ > strlcat(message, copy->url, sizeof(message)); > DrawUI(This->netscape_widget, local_td->instance, message, 0, -1); > if (copy->play) { > - strlcpy(command, cmd, sizeof(command)); > + > // for rtsp streams we need to specify FPS > if (strncmp(copy->url, "rtsp", 4) == 0) { > - strlcat(command, " -fps ", sizeof(command)); > - strlcat(command, " 30 ", sizeof(command)); > + argv[argc++]="-fps"; > + argv[argc++]="30"; > } > if (copy->playlist == 1) > - strlcat(command, " -playlist ", sizeof(command)); > - strlcat(command, "\'", sizeof(command)); > + argv[argc++]="-playlist"; > > if (copy->mmsstream) { > - remove_quotes(copy->url); > - strlcat(command, copy->url, sizeof(command)); > + argv[argc++]=copy->url; > if (This->keep_download) { > snprintf(mmsplaylist, sizeof(mmsplaylist), > "%s/playlist", This->download_dir); > @@ -1272,18 +1305,14 @@ > pthread_mutex_unlock(&(This->playlist_mutex)); > continue; > */ > - strlcat(command, copy->url, sizeof(command)); > + argv[argc++]=copy->url; > } else { > - strlcat(command, copy->fname, sizeof(command)); > + argv[argc++]=copy->fname; > } > } > - strlcat(command, "\' <> ", sizeof(command)); > - mkfifo(This->fifoname, S_IWUSR | S_IRUSR); > - strlcat(command, This->fifoname, sizeof(command)); > > if (DEBUG) { > printf("URL: %s\n", copy->url); > - printf("CMD: %s\n", command); > } > > if (!copy->mmsstream) { > @@ -1304,7 +1333,9 @@ > } > } > > - player = popen(command, "r"); > + argv[argc++]=NULL; > + player = mypopen(argv, &This->pid, &This->control); > + > This->state = STATE_PLAYING; > if (player != NULL) { > while (fgets(buffer, sizeof(buffer), player) != NULL) { > @@ -1363,13 +1394,12 @@ > break; > } > } > - pclose(player); > + > + fclose(This->control); > + fclose(player); > + You probably have to waitpid() here instead of signal(SIG_IGN) here... ...Also, shouldn't This->pid be set to 0 after this? Otherwise you could end up killing a random pid if the pid is reallocated.. Cheers, Dave |