(In reply to comment #8) > (In reply to comment #6) > > What is MOZ_NO_CONFIG? > > I have no idea. Where does this appear? @@ -12371,6 +12274,9 @@ MOZ_NO_ACTIVEX_SUPPORT=1 MOZ_NO_INSPECTOR_APIS= MOZ_NO_XPCOM_OBSOLETE= MOZ_NO_FAST_LOAD= +MOZ_OGG=1 +MOZ_VIDEO=1 +MOZ_NO_CONFIG= > > What state is your tree in? "media" is not present on trunk. > > See the bit in the my comment about the patch requiring the 'third party > library' patch to be applied first: > > http://www.double.co.nz/video_test/third_party_modules.patch.gz Oh I see. I'd assumed that only created new files and didn't patch Mozilla files. It would probably be good to do it that way: one patch that simply creates all files for the non-Mozilla libraries and another patch that patches Mozilla files and creates new Mozilla files. > > We'll need to figure out a final destination for portaudio and the Ogg > > libraries. "content" isn't it. > > How about in the root 'modules' directory, where libbz2, etc are? If that is > the preferred place I'd need some advice on how to have 'content/media/video' > have that as a dependancy. I'm not sure. We should ask bsmedberg on IRC or CC him here. > > -- Keep a reference to pending events from the content element... > > To do this I'll need to keep a collection of events. What's the best way of > storing an array/list of events? Is there a collection class that can safely > hold nsCOMPtr's? Are you sure you need multiple events of the same type pending simultaneously? Which event type(s) would that be? Often it's simpler to just have one event which pulls the "next thing to do" off some internal queue. > If the events are posted to the queue of a thread which is not the main > thread, is it sufficient to stop those threads - therefore the remaining > events won't be processed? That's they way I'm currently handling it. Remaining events are always processed. I believe thread shutdown waits for all events to be processed before returning. I guess this does work, although it would stop working if we start sharing threads between videos, and it might impose significant delay when we tear down a video element. > > It seems that you have threads other than the main thread touching content > > elements and possibly even frames. > > All frame stuff is done on the main thread. The only thread code that affects > the frame is via an event dispatched to the main thread. I do call > GetPrimaryFrame in that non-main thread though, to pass to the invalidate > event. I'll move to passing the element to the event and have that call > GetPrimaryFrame so it occurs in the main thread. Is that ok? That's a good plan. You will have to deal with revoking the invalidate event when the element goes away (or holding a strong ref to the element from the invalidate event). > No element code is touched on other threads except for specific members I > added, so I'm pretty sure nothing in the core code is being called off the > main thread. I do plan to move that code out into another object though to > seperate it from the element. Yeah, you might get away with it, but it still smells bad :-) > > You generally do not want to pass nsCOMPtrs as parameters. Just take an > > nsIThread* here. > > They are references to nsCOMPtr's and being assigned in the constructor > initialisation to another nsCOMPtr. If I make it a raw nsIThread*, but my > original pointer is an nsCOMPtr, is it safe to do: > > void var(nsIThread* baz) { ...store baz in an nsCOMPtr for later use... } > nsCOMPtr foo = ...; > call_bar(foo); > > Can I pass that foo directly or do I need to call a method on nsCOMPtr to get > at the pointer? nsCOMPtr auto-converts to a raw pointer in this context, so no extra syntax is required. It's safe. > > You'll need to > > use operator new instead of PR_Malloc, which is also a good thing :-) > > I used operator new in patch1 but another commenter via email suggested using > PR_Malloc :-) What's a good strategy for protecting against integer overflow > here? Given that the video size is from the ogg file, a manipulated ogg could > craft an overflow. I see. I would suggest an explicit size check here. A gigantic memory allocation might not fail but just bring the user's system to a grinding, thrashing halt. Also, you need to check for integer overflow here; generally speaking integer multiplications in an allocation size expression are bad. > > Why are you doing this while painting? Is this the only time you can get the > > video height/width? Can the video height and width change during the stream > > or something? > > Yes, the height and width can change during the stream. Exciting! > I get the video height > and width when decoding the video frame data. So I'd be doing it at the same > time as I send the event that does the invalidate on the frame. Would it be > better to do it there (in the nsHTMLVideoElement code, just before I call > Invalidate)? Yes. I think it's safe to request a reflow while painting, but it's unusual and best avoided. Also, here requesting reflow during painting is too late: you'll paint a video image when the frame has the wrong size, before it gets resized to the right size. Requesting the reflow when you invalidate should work much better, we'll try to flush reflows before the paint happens so the frame will switch to the right size. There could still be problems where the image we decide to draw isn't the one we saw at invalidate time (because of A/V synch), and therefore still might be the wrong size for the frame, but we can't fix that issue without the compositor. Then again I've never seen a video that changed size halfway through :-).