GNOME Bugzilla – Bug 582442
Set video preview on top of remote video
Last modified: 2011-07-12 15:08:30 UTC
I find it a bit ugly to have the video preview on the right of the remote video. iChat, afaik, set the preview in the bottom right of the remote video, that looks much better. This is especially true for the fullscreen mode since we don't have preview at all atm.
Branch containing an almost-done-work-in-progress proposed fix: http://git.collabora.co.uk/?p=user/jtellier/empathy.git;a=shortlog;h=refs/heads/video-preview-overlay
Created attachment 138537 [details] [review] Proposed fixed in branch http://git.collabora.co.uk/?p=user/jtellier/empathy.git;a=shortlog;h=refs/heads/video-preview-overlay
So X seems to have started killing Empathy when I made Video calls (hooray), unrelated to this patch. So I can't test it... but at least I noticed that transparent PNGs on the front window are transparent onto the background window. Maybe I'm thinking about this wrong, but should we be combining this video together in GStreamer instead of X?
The problem with that approach is that it makes the presentation of the video preview dependant of the remote user. If we are not receiving any video, the video preview is drawn over the remote avatar, not over another video stream. I also have not seen drop in performance in making X show the two video stream over each other (but have not tested by doing it the GStreamer way). This makes me think that what you have suggested might complicate thing without giving us many benefits. Does this make sense or have I missed something?
From Daniel Stone, eminent X.org hacker: <daniels> cassidy: putting two videos on top of each other doesn't work reliably -- last i checked, the omap display controller would just wedge if you tried <daniels> cassidy: sadly, it's much much safer to mangle them together in gstreamer and then hand x one video stream, at least until overlays are dead and we're all living in a glorious textured video future (and/or yellow submarine) <cassidy> daniels: so your vote is "we should do it in gst"? Or should we wait? <daniels> cassidy: well, do it in gst, or wait five years :)
I'm not sure that we should overlap the preview in the not-fullscreen mode. The preview is hidding a big part of the remote's video. Furthermore, with your current implementation, we now waste a lot of space: http://people.collabora.co.uk/~cassidy/call-ui-waste.png
I suggest showing your video in the corner of the other party's video, but with a button in the corner to toggle the visibility of your video -- much like the button to toggle the overview in the corner of a Google Maps map. I can't comment on the space wasting, because I don't know what the "Sidebar" button in that screenshot shows or hides. It does seem rather weird that Empathy should be showing you a numeric keypad when you're already having the call.
(In reply to comment #7) > I can't comment on the space wasting, because I don't know what the "Sidebar" > button in that screenshot shows or hides. It shows and hides the sidebar which contains either a dailpad, audio volume sliders, or video colour balance controls, based on the drop-down at the top of it. > It does seem rather weird that > Empathy should be showing you a numeric keypad when you're already having the > call. If you've made a SIP call to a phone booking service, you can use the dialpad to choose options from menus or type in your credit card number, etc.
Ok, a sidebar doesn't seem like a good fit for any of those functions. Perhaps that can be addressed in a separate bug report. In the meantime, a simple way of reducing the space taken by the button would be to put it below the video instead of beside it.
Correct me if I'm wrong, but the reason my patch works is because I use a GtkEventBox which provides its own window. Since the video output and the video preview are in distinct windows, they can be on top of each other without any problem. I was not thinking about this issue when I worked on this branch, so I guess I've avoided the problem by accident. If this is correct, is there something wrong in having a separate window for each video stream? It might not be the cleanest way of doing it, but does doing so introduces any drawbacks? I could always re-implement everything using gstreamer, but before doing so, I want to be sure that what I have done is truly inaccurate.
Oh, and by the way, Guillaume, the space wasted which you have pointed out has nothing to do with this branch. I don't recall which branch introduced that, but for some time now, when no video is being sent, the video preview is hidden by default. I even think that you're the one who asked me for that feature on IRC :)
Review of attachment 138537 [details] [review]: Plan is now to implement this using Clutter.
Branch implementing this: http://cgit.collabora.com/git/user/pochu/empathy.git/log/?h=call-preview-582442 There are still a few things to tweak, but I think we can go and merge this and fix issues / improve the UX later (this only affects empathy-call and not empathy-av). Some things that still need fixing: move sidebar button? sync/async flip video preview better background color? Suggestions for what to do with the sidebar / sidebar button welcome.
Your branch doesn't build: empathy-call-window.c: In function ‘empathy_call_window_init’: empathy-call-window.c:1127:3: error: ‘video_container’ undeclared (first use in this function) empathy-call-window.c:1127:3: note: each undeclared identifier is reported only once for each function it appears in +on_stage_allocation_changed (ClutterActor *stage, Please add a comment explaining the logic here, it's not that clear. (In reply to comment #13) > sync/async How does this affect the call? > Suggestions for what to do with the sidebar / sidebar button welcome. Maybe we could get rid of the button and only use the menu to display the sidebar? If we do that I'd add a toolbar button to get the dialpad as that's what most people will use (and no one seems to find it atm anyway).
(In reply to comment #13) > better background color? Can't we get the background color from the GTK+ theme? It looks pretty weird atm.
(In reply to comment #13) > sync/async > flip video preview These two are now fixed (the sync/async one in the "CallWindow: use clutter to draw the video preview" commit, the flip video in a new commit). (In reply to comment #14) > Your branch doesn't build: > empathy-call-window.c: In function ‘empathy_call_window_init’: > empathy-call-window.c:1127:3: error: ‘video_container’ undeclared (first use in > this function) > empathy-call-window.c:1127:3: note: each undeclared identifier is reported only > once for each function it appears in Oops, bad squashing before pushing the branch, fixed. > +on_stage_allocation_changed (ClutterActor *stage, > Please add a comment explaining the logic here, it's not that clear. Actually that's not needed at all, so I've removed it. > (In reply to comment #13) > > sync/async > > How does this affect the call? I'm not sure, but I've fixed it now (Sjoerd says it's still needed). > > Suggestions for what to do with the sidebar / sidebar button welcome. > > Maybe we could get rid of the button and only use the menu to display the > sidebar? If we do that I'd add a toolbar button to get the dialpad as that's > what most people will use (and no one seems to find it atm anyway). I was going to add a button when I added the menu entry, but I didn't because we have no suitable icon... I can add a Call->Sidebar menu maybe (or let people use the Dialpad menu) for now. (In reply to comment #15) > Can't we get the background color from the GTK+ theme? It looks pretty weird > atm. Yeah it does :( I'll try that.
(In reply to comment #15) > Can't we get the background color from the GTK+ theme? It looks pretty weird > atm. Fixed in the "CallWindow: set the video container background color" commit.
(In reply to comment #14) > Maybe we could get rid of the button and only use the menu to display the > sidebar? If we do that I'd add a toolbar button to get the dialpad as that's > what most people will use (and no one seems to find it atm anyway). Added a commit to do that (sidebar button removed, one can access the sidebar through the "Dialpad" menu). I'll eventually add a tool button for the dialpad, but I need to request/design an icon first.
I'm not sure we need "Disable Camera" on the preview, we can easily do it using the toolbar. Removing it would remove the context menu which would be better, I think. This button on the preview looks pretty shit :) Can't we do something sexier using Clutter magic? Like having the icon displayed as an overlay on top of the video? "Dialpad" should be in View, not Call. Actually I think I'd rename it "Sidebar" instead, opening the sidebar with the last page displayed when the sidebar was hidden (defaulting on the dialpad for the first time). Can you please ask the art guys for a dialpad icon?
(In reply to comment #19) > I'm not sure we need "Disable Camera" on the preview, we can easily do it using > the toolbar. Removing it would remove the context menu which would be better, I > think. I've done what the mockups have :) They also have this "Switch camera" menu, so if we are going to implement it, removing "Disable camera" is not going to help, so I'd leave it. > This button on the preview looks pretty shit :) Can't we do something sexier > using Clutter magic? Like having the icon displayed as an overlay on top of the > video? It indeed looks fugly :( I did that to get something working first, I'll try to make something nicer soon. What icon are you thinking about? > "Dialpad" should be in View, not Call. Indeed, I'll fix that. > Actually I think I'd rename it "Sidebar" > instead, opening the sidebar with the last page displayed when the sidebar was > hidden (defaulting on the dialpad for the first time). I can do that, but not until we have a dialpad icon so that I can add a tool button (otherwise it's impossible to find the dialpad). > Can you please ask the art guys for a dialpad icon? Will do!
(In reply to comment #20) > (In reply to comment #19) > > I'm not sure we need "Disable Camera" on the preview, we can easily do it using > > the toolbar. Removing it would remove the context menu which would be better, I > > think. > > I've done what the mockups have :) They also have this "Switch camera" menu, so > if we are going to implement it, removing "Disable camera" is not going to > help, so I'd leave it. Good point; I didn't look at the mockups since a while so kinda forgot about it. :) > > This button on the preview looks pretty shit :) Can't we do something sexier > > using Clutter magic? Like having the icon displayed as an overlay on top of the > > video? > > It indeed looks fugly :( I did that to get something working first, I'll try to > make something nicer soon. What icon are you thinking about? My idea was displaying the camera-web icon directly on the video as the goal of this menu is to control the camera I think that makes sense and maybe looks better. > > Actually I think I'd rename it "Sidebar" > > instead, opening the sidebar with the last page displayed when the sidebar was > > hidden (defaulting on the dialpad for the first time). > > I can do that, but not until we have a dialpad icon so that I can add a tool > button (otherwise it's impossible to find the dialpad). I don't think we should block on the icon. Best to get the basic UI implemented ASAP so we can test it to see how usable it is. Use a crappy icon for now (accessories-calculator as it kinda look like a dialpad if you're high? :D), as long as we have the proper icon for 3.2 that's fine.
(In reply to comment #21) > I don't think we should block on the icon. Best to get the basic UI > implemented ASAP so we can test it to see how usable it is. Use a crappy icon > for now (accessories-calculator as it kinda look like a dialpad if you're > high? :D), as long as we have the proper icon for 3.2 that's fine. You really need to be high for it to look like a dialpad :P but let's use that for now, we should get an icon in gnome-icon-theme soon, and if not we can put it in Empathy, see bug #654230. Commit added to add the toolbar button and change the menu entry.
I've added three more commits to fix bug #634809 and a couple of related issues (I haven't done them in master as they would cause conflicts and I want to merge this soonish). Are you OK if we merge this now and make the preview icon nicer afterwards?
Do we really need to catch empathy_call_window_configure_event_cb? We only use the stored value when destroying; can't we just get these values by then? Or maybe it's too late?
(In reply to comment #24) > Do we really need to catch empathy_call_window_configure_event_cb? We only use > the stored value when destroying; can't we just get these values by then? Or > maybe it's too late? I tried that but it's too late, we can get the width and height, but the x and y coordinates are 0 by then.
Merged! I'll open another bug report for the button improvements.
*** Bug 653449 has been marked as a duplicate of this bug. ***