Created attachment 63013 [details] [review]
Michael Rickmann's solution

It looks pretty good. I have a few questions still :

1) what is this #ifdef WIN32_FULLSCREEN ?

2) why is while (SDL_PollEvent (&event)) {} WIN32-only ? If it is something that has to be done, perhaps it would be worth to have it unconditionally ?

3) I'm a little annoyed that there is a difference of interpretation for display_type and fs_active depending on the platform... perhaps we could do the g_idle_add trick on all platforms, to limit the amount of unshared code ? [Damien may complain about it... let's wait and see what he says]

ad 1): SDL_WM_ToggleFullScreen is not available for WIN32. The reason and solution for that are explained in the SDL for Windows faq at http://www.libsdl.org/faq.php?action=listentries&category=4#79. At the moment we do not have a "real" SDL-fullscreen in Windows if we do not compile with -DWIN32_FULLSCREEN but instead a SDL window with the size of the screen. The real fullscreen is a hardware surface which uses DirectX, and if a crash occurs it is terribly difficult to recover without rebooting the box. Additionally you would not be able to test Win32 Ekiga with VNC. So basically it is commenting out the advanced feature to give people a chance to test and survive a crash.
ad 2): I have been very cautious not to change anything for Linux because I have not tested it. Certainly it cannot hurt Linux to empty the SDL event queue completely.
ad 3): The g_idle trick reacts sometimes rather slowly and there are cases when the SDL window pops up after a second and gets filled after two seconds. So for Linux that would make things worse. Another thing is code unification in gm_main_window_update_video. There are already several variables dealing with fullscreen: mv->screen, fs_active and display_type. So I reused them.
In priciple anything which works asynchronously should work for the synchronous mode as well. We should forget about fs_active and always use mv->screen as you have already done after the SDL_CreateRGBSurfaceFrom .... That would make the code cleaner. As to the semantics of display_type, let us take out that comment of mine. Both Linux and Windows take it as a request initially and as a type later during the routine. If that is not applicable Windows bails out. That part of the code could become:
if (display_type != FULLSCREEN) {
  gm_mw_destroy_fullscreen_video_window (main_window);
  if (mv->screen)
    return;
}
Linux would stay in, Windows would return as long as the SDL video is alive.
Regards
Michael

Well, somehow I like this threading issue. As it comes to unifying code for Linux and Windows I have made another patch which is a bit more intrusive but runs well on Windows and Linux as I have checked. I have changed gm_mw_init_fullscreen_video_window and gm_mw_destroy_fullscreen_video_window to gboolean funcs(void *data). So they can be submitted to g_idle_add but may be also called directly. This is done through a macro CALL_OR_IDLE_ADD which handles things differently for Linux and Windows. For the SDL_Init and SDL_Quit I can use for Windows the same as it has been for Linux. SDL_WM_ToggleFullScreen does not have any effect in win32, so I added SDL_FULLSCREEN to the flags passed to SDL_SetVideoMode. In gm_main_window_update_video I moved the setting of the helper booleans behind the switching (fs_active alone would have done) and set fs_active acording to the result of switching. This way, Windows does not have to bail out after a destroy request but can draw to the SDL surface until it gets destroyed.
Regards
Michael

Created attachment 63302 [details] [review]
minimize different code between Linux and Windows

Instead of void* it should be gpointer.

For the #ifdefs, I'd rather see :
#ifdef WIN32
<all win32 defines>
#else
<all non-win32 defines>
#endif
(ie: no playing with #undef)

Instead of doing the SDL_Init in the g_return_val_if_fail, I would have a gboolean result, then result = SDL_Init, then g_return_val_if_fail (result not good, ...).

A switch(display_type) would be nicer than the forest of if...

Damien, what do you think about it ?

Damien has no time to look into this for now...

Michael, what's the status of this ?

I use the last patch with Win32 as it is and it works very well, but it changes Linux code. I admit that I have not tested it extensively under Linux. The first patch does not change things for Linux but it looks ultimatly ugly. Because of that it would not stay in the code for very long. I wanted to wait until Ekiga 2.02 is released and see wether things become more relaxed after that. On the other hand, now, I might be able to convince Kilian to include it into the Win32 build process, because Damien does not have time and we could have it tested by more users before we molest Damien again. Actually I would prefer this approach.
Regards
Michael

Well, I was more asking Damien if I wasn't too strict in the changes I asked you than a real review ;-)

I would commit it even before 2.0.2, since it closes a bug :-)

I'll be away for a few days soon anyway.

I have beautified the last patch following your suggestions. I left the forest of if in place though. I tested it on Windows extensively and also shortly on Linux. It works. So its a small solution not a great overhaul.
Regards
Michael

Created attachment 64454 [details] [review]
beautified version of last patch

Looks good, committed. Thanks!