Re: [Mplayerplug-in-devel] Repeat plays & thread management...

[Nick P. <Nic...@st...>]

On 15/06/2005, at 1:32 AM, Kevin DeKorte wrote:
>
> Nick,
>
> Well I took a look at this patch and you removed a few lines of  
> code that are
> needed.

I thought I probably would, but it's really hard to tell what the  
purpose of some parts of it is.

>
> Looks like you removed the ability to have a media that is  
> autostart=false and
> then be started when the play button is clicked.

No, that's what mine is, and it works.

> The reason controlwindow is in there is for the case where the user  
> is in a X
> widget mode and does not have a button to click.

Ah, right. Like I said, I want to make sure I'm not making things  
worse before I go on trying to make them better...

> Looks like you also moved some code around.. lets see if we can get  
> a cleaner
> patch and get this solved.

Yes. I think it makes more sense if it can be moved into a more  
logical order; the tests around each block were really ugly and error- 
prone. If it can be re-ordered so the tests become more intuitive and  
less complex, I reckon that would be a really good thing.

Essentially it seems like it makes sense to go through the blocks  
(and tests) in that method in the order that they would be performed  
to start the thing up in the first place - thread setup first, if  
needed, followed by launching the player, making sure the playlist is  
set up, signaling the thread, whatever.

I would also suggest making the functions that are called from there  
consistent in either requiring or not requiring that the control  
mutex be held on entry -- signalPlayerThread and launchPlayerThread  
being the most glaring examples here. Since I wasn't familiar enough  
with the code to be confident I understood exactly what the mutex was  
protecting against and where those functions would be called from, I  
didn't touch any of that.

The other thing that struck me as worth considering is whether or not  
the player thread really ought to exit when it finishes the playlist;  
I guess so long as the thread handling code in the main thread is  
simple enough and can reliably restart it, that's not a problem.  
Letting it exit is probably better on balance, as it protects against  
any problems caused by mozilla not cleaning up properly when the page/ 
tab is closed, or a new page loaded. We have had problems in this  
area too, but apparently only when the new page is loaded by  
submitting a form (haven't got to the bottom of this yet).

Happy to help as much as I can, anyway.


Cheers,


Nick