Created attachment 222172 [details] [review]
videooutput-manager-x: refactor setup_frame_display ()

The creation of a Windows is centralized in the new method create_window(),
avoiding code repetition and more readable.

I don't like that init_cont structure very much :

1. it doesn't have a very explicit name -- what is that "cont" suffix about?

2. its memory management isn't clean, in as much as it's currently a local variable which gets passed by reference (and a non-const), then its data gets retrieved, then it goes out of scope and hence disposed of -- that looks okish until that point, but it doesn't look foolproof for the future ; I'd rather see either more function parameters or a full-blown boost::shared_ptr-managed structure to pass around.

I must admit that I don't know that video display code much ; I had always hoped that it would die replaced by a gstreamer-based code which would give less headaches (would take care of acceleration if available, of portability to win32 and mosx)... Hélas, I never managed to get my experimental gstreamer code to work straight for devices already, so never pushed for more.

(In reply to comment #2)
> I don't like that init_cont structure very much :
> 
> 1. it doesn't have a very explicit name -- what is that "cont" suffix about?

It comes from the "continuation" concept: it represent the state to be passed to to the Windows.Init() method.

> 2. its memory management isn't clean, in as much as it's currently a local
> variable which gets passed by reference (and a non-const), then its data gets
> retrieved, then it goes out of scope and hence disposed of -- that looks okish
> until that point, but it doesn't look foolproof for the future ; I'd rather see
> either more function parameters or a full-blown boost::shared_ptr-managed
> structure to pass around.

IMO there's no need to keep the structure in memory for more than its local scope. The values of the structure are copied as soon as the .Init() method is called, and there's no need to keep them afterwards.

Now, I realize that the code is too C-ish, rather than C++-ish. I'm used to think in C, not in C++ :(

> 
> I must admit that I don't know that video display code much ; I had always
> hoped that it would die replaced by a gstreamer-based code which would give
> less headaches (would take care of acceleration if available, of portability to
> win32 and mosx)... Hélas, I never managed to get my experimental gstreamer code
> to work straight for devices already, so never pushed for more.

Yes. I do agree: a gstreamer-based video display would simplify the code a lot. But my current task is to finish the bug #681310

It introduces a regression: PIP and fullscreen are not working, the remote video is shown instead of both of them.

(In reply to comment #4)
> It introduces a regression: PIP and fullscreen are not working, the remote
> video is shown instead of both of them.

F$%&!

I'm deeply sorrow Eugen. Thanks for taking care.

Sadly, I won't be able to work on this until the end of September (linuxcon && holidays! \o/)

Thanks again!

No problem.  Hope it will be for ekiga 4.0.0 then!

Created attachment 223387 [details] [review]
videooutput-manager-x: refactor setup_frame_display ()

The creation of a Windows is centralized in the new method create_window(),
avoiding code repetition and more readable.

I've uploaded a new revision of the patch.

Now it handles the PIP and full-screen use-cases correctly, as fas as I tested.

It removes ~30 loc and removes a state in Ekiga::VideoOutputAccel

Created attachment 223390 [details] [review]
videooutput-manager-x: refactor setup_frame_display ()

The creation of a Windows is centralized in the new method create_window(),
avoiding code repetition and more readable.

Committed, thank you very much!  http://git.gnome.org/browse/ekiga/commit/?id=88edad76f3de80.