[prev in list] [next in list] [prev in thread] [next in thread] 

List:       helix-video-dev
Subject:    [hxvideo] Re: CVS diff for the guiplayer issues
From:       "Greg Wright" <gwright () real ! com>
Date:       2003-09-30 18:52:55
Message-ID: 093801c38784$0faf37c0$7c6f17ac () gwright3
[Download RAW message or body]

From: "Sunil Srivastava" <ssrivastava@real.com>
> 
> Hi Greg
> 
> Thanks for your detailed review comments. From your mail, it seems that for 
> guiplayer related issues you want changes
> in the guiplayer code. 

It is not required that all changes be made in guiplayer code to fix
guiplayer problems. If the bug lies outside of the guiplayer code
then it does make sense to make changes elsewhere. No problem there.

> Once the changes for windows are finalized, I will 
> check for the other platforms also. 

I think that if you look at more then one platform at a time you
can save some work. For example, if the SameWindow() function is
going to be needed in linux as well as windows then we don't want 
to define it in winsite.cpp, but rather in the cross platform 
basesite.cpp.That kind of thing. That is how I would approach 
the problem anyway.

> Corresponding to
> each changes, I have added my comments into the diff file.  Is it possible 
> for you to review the other changes also? In some
> cases, it seems difficult to move the changes in guiplayer as child window 
> is created in winsite when HELIX_SAME_WINDOW=0
> and all the messages from mouse click is handled by the WindowProc in winsite.

I guess I am little confused about this HELIX_SAME_WINDOW to
begin with. Why do we need it? The site already has information
about who created the window it is using, and so it isn't needed 
there: m_bWindowCreatedByCreate in basesite.cpp. I see that that
env var controls whether or not guiplayer creates the window itself
or lets the site do it. Why do we need guiplayer to do both? All
TLCs written so far just choose which method works best for them
and sticks with that one way of doing it. If there is some bug
that makes us want to switch back and forth then we should fix 
that.

If you are experiencing crashes while entering/exiting full screen
mode  I believe, but don't know for sure, that it is being caused 
by the reparenting of the playback window by the site when full 
screen mode is entered or exited. If this is the case I can help
fix that.

The rest of the changes below seem be solving GUI TLC problems like
'cross button not working', 'we want to exit on left button click'
and 'we want to hide the stop/pause buttons while in fullscreen'.
These all seem like problems that should be solved in the TLC code,
not the site. The site, as it sits, works for several other TLCs, 
like the Window's RealOne player. Perhaps you should add your own
message loop to the window and sniff keystrokes there? Perhaps
you need an additional window. One that can be used as the parent
for the window you pass into the site (or that the site creates
for you).

I am not exactly sure what is motivating all these changes (is 
there a bug number I can look at?) but I would guess that a good
first step would be to decide if the guiplayer is going to create
the playback window(s) or if the site is going to. Then, fix the
problems given that one type of playback scenario and not worry 
about the other. It just adds complexity you don't need. My 
suggestion would be to create you own window, thereby having more
control over it. You can have your own message loop for that window
and enter/exit full screen as you wish. If you do create your own
window, make it a child of a 'parent' window so you don't run into
any reparenting issues. I could see that we might have issue with
the site now doing the correct thing regardless of whether or not
it has a proper parent window but that can be a separate bug.

If you provide the original bug# or what you are trying to fix
I can be of more help.

--greg.




> Index: winsite.cpp
> ===================================================================
> RCS file: /cvs/video/sitelib/platform/win/winsite.cpp,v
> retrieving revision 1.4
> diff -u -w -r1.4 winsite.cpp
> --- winsite.cpp 25 Feb 2003 19:27:14 -0000 1.4
> +++ winsite.cpp 27 Sep 2003 11:23:00 -0000
> @@ -132,6 +132,21 @@
> }
> 
> 
> +namespace
> +{
> +    inline bool SameWindow()
> +    {
> +        // XXXSAB: pick a reasonable config method...
> +        const char* envarSameWindow = getenv("HELIX_SAME_WINDOW");
> +        if (envarSameWindow &&
> +            (strcmp(envarSameWindow, "0") == 0 ||
> +             strcasecmp(envarSameWindow, "no") == 0 ||
> +             strcasecmp(envarSameWindow, "false") == 0)) return false;
> +        return true;
> +    }
> +};
> +
> +
> /************************************************************************
> *  Method:
> *  Constructor
> 
> [Greg]
> 
> environmental variables aren't really what we use for this kind of 
> config options. We generally use the registry for this type of thing,
> and we should in this case as well to keep things consistent.
> 
> [Sunil]
> 
> Can we keep the SameWindow() concept in winsite.cpp or not, either through Regisry
> or through environment variable. As this is being used by multiple player, if \
> required the default value can be set accordingly.Please suggest
> 
> 
> @@ -1181,6 +1196,8 @@
> break;
> 
> case WM_LBUTTONDOWN:
> +           if (m_bInFullScreen)
> +        _ExitFullScreen ();
> #ifdef _DEBUG
> if( wParam & MK_CONTROL )
> {
> 
> 
> [Greg] Again, we are changing the behavior of all windows builds
> here. Some TLCs want to decide what keystroke should be used
> to exit full screen mode. Perhaps this should be moved into
> the WindowProc of the guiplayer?
> 
> [Sunil] When we are in full screen mode and HELIX_SAME_WINDOW=0,currently 
> there is no option in the guiplayer to exit or stop the clip. User has to
> wait till the clip ends. Please see if this is required or not. Also currently 
> when HELIX_SAME_WINDOW=0, WM_LBUTTONDOWN message is not being handled by the 
> guiplayer WndProc.
> 
> 
> @@ -1228,6 +1245,11 @@
> case WM_KEYDOWN:
> case WM_KEYUP:
> case WM_NCHITTEST:
> +           if (message == WM_KEYDOWN)
> +    {
> +     if (wParam == VK_ESCAPE && m_bInFullScreen)
> +       _ExitFullScreen ();
> +    }
> SetEvent(event, message, (void*)hWnd, (void*)wParam, (void*)&lParam);
> break;
> 
> [Sunil] Same as above
> 
> 
> 
> @@ -1295,7 +1317,7 @@
> #endif
> case WM_CLOSE:
> {
> -           if (m_bInFullScreen && m_pUser)
> +           if ((m_bInFullScreen && m_pUser)&&(!SameWindow()))
> {
> HXxEvent event = {WM_CHAR, GetWindow()->window, (void*)VK_ESCAPE, 0, 0, 0};
> m_pUser->HandleEvent(&event);
> 
> [Greg] Since SameWindow() defaults to TRUE, this is changing the default behavior \
> for all  window's builds. Same with more of the diffs below.
> 
> [Sunil] After getting your feedback for point number-1, we will implement \
> accordingly. This has been added because when HELIX_SAME_WINDOW=1, and we are in \
> full screen mode the cross button is not responding 
> @@ -1563,7 +1585,7 @@
> {
> HWND retVal = SetParent( (HWND) GetWindow()->window, NULL);
> }
> -    else
> +    else if(!SameWindow())
> {
> m_pContainingWindow = new CHXWinFullScreenWindow(this);
> m_pContainingWindow->Create();
> 
> [Sunil] We feel that when HELIX_SAME_WINDOW=1, it is not required to create
> the child window
> @@ -1618,7 +1640,7 @@
> }
> else
> {
> -        SafeMoveWindow((HWND) GetWindow()->window, 0, 0, newsize.cx, newsize.cy, \
> FALSE); +        SafeMoveWindow((HWND) GetWindow()->window, 0, 0, newsize.cx, \
> newsize.cy, TRUE); }
> 
> /*
> 
> [Sunil] To refresh the player screen when entering in full screen \
> mode(HELIX_SAME_WINDOW=1) @@ -1643,7 +1665,7 @@
> CHXMapPtrToPtr::Iterator i = zm_ListOfHiddenWindows.Begin();
> for(;i!=zm_ListOfHiddenWindows.End();++i)
> {
> -            SafeShowWindow((HWND) i.get_key(), SW_SHOW);
> +            SafeShowWindow((HWND) i.get_key(), SW_HIDE);
> }
> zm_ListOfHiddenWindows.RemoveAll();
> }
> [Sunil] To hide the buttons (STOP/PAUSE etc)when entering in full screen mode \
> (HELIX_SAME_WINDOW=1) 
> 
> @@ -1682,7 +1704,7 @@
> 
> 
> HWND retVal = SetParent((HWND)GetWindow()->window, m_windowParent);
> -    if (!zm_bInFullScreenTest)
> +    if ((!zm_bInFullScreenTest)&&(!SameWindow()))
> {
> m_pContainingWindow->Hide();
> m_pContainingWindow->Destroy();
> [Sunil] We added this check because when HELIX_SAME_WINDOW=1, child window was not \
> created in full  screen mode
> @@ -1691,7 +1713,7 @@
> 
> SafeSetWindowPos((HWND) GetWindow()->window, HWND_TOP, 0,0,0,0,SWP_NOMOVE  | \
> SWP_NOSIZE ); 
> -    SafeMoveWindow((HWND) GetWindow()->window, m_windowPosition.x, \
> m_windowPosition.y, m_windowSize.cx, m_windowSize.cy, FALSE); +    \
> SafeMoveWindow((HWND) GetWindow()->window, m_windowPosition.x, m_windowPosition.y, \
> m_windowSize.cx, m_windowSize.cy, TRUE); 
> m_pTopLevelSite->m_bDisableForceRedraw = FALSE;
> memset(&m_screenOffset, 0, sizeof(HXxPoint));
> 
> [Sunil] To refresh the desktop window when exiting from full screen \
> mode(HELIX_SAME_WINDOW=1) 
> 
> 
> Index: basesite.cpp
> ===================================================================
> RCS file: /cvs/video/sitelib/basesite.cpp,v
> retrieving revision 1.13
> diff -u -w -r1.13 basesite.cpp
> --- basesite.cpp 19 Sep 2003 22:03:27 -0000 1.13
> +++ basesite.cpp 29 Sep 2003 10:14:55 -0000
> @@ -2729,6 +2729,11 @@
> 
> _TLSLock();
> 
> +    if( m_pVideoSurface )
> +    {
> +     m_pVideoSurface->EndOptimizedBlt();
> +    }
> +
> if (m_pUser && m_bDetachWndMsgPending)
> {
> // send HX_DETACH_WINDOW msg to the renderers
> 
> [Sunil] To remove the memory leak when HELIX_SAME_WINDOW=1
> 
> 
> @@ -2780,6 +2785,7 @@
> }
> }
> 
> +
> _TLSUnlock();
> UnlockBlitters();
> 
> @@ -2841,10 +2847,6 @@
> //This could be the site that owns the overlay even though
> //it doesn't have a window. Give it a chance to clean up
> //here...
> -    if( m_pVideoSurface )
> -    {
> -        m_pVideoSurface->EndOptimizedBlt();
> -    }
> 
> _DestroySliders();
> 
> [Sunil] This is being handled in DetachWindow()
> 
> 
> 
> 
> Index: chelixwindow.cpp
> ===================================================================
> RCS file: /cvs/clientapps/guiplayer/chelixwindow.cpp,v
> retrieving revision 1.4
> diff -u -w -r1.4 chelixwindow.cpp
> --- chelixwindow.cpp 29 Aug 2003 20:51:22 -0000 1.4
> +++ chelixwindow.cpp 27 Sep 2003 11:24:26 -0000
> @@ -601,6 +601,7 @@
> }
> 
> RequestResize(m_szIdeal);
> + Layout ();
> }
> }
> 
> [Sunil] To refresh the player screen when exiting from full screen mode to original \
> mode 
> @@ -618,6 +619,7 @@
> 
> HXxSize szDouble = { m_szIdeal.cx * 2, m_szIdeal.cy * 2 };
> RequestResize(szDouble);
> + Layout ();
> }
> }
> 
> [Sunil] To refresh the player screen when exiting from full screen mode to double \
> size mode 
> @@ -652,6 +654,7 @@
> OnPostFullScreen();
> }
> }
> +    Layout ();
> }
> 
> [Sunil] To refresh the player screen when the player exits from full screen mode \
> after completing the clip
> 
> 
> 
> Index: chelixwindowwin.cpp
> ===================================================================
> RCS file: /cvs/clientapps/guiplayer/win/chelixwindowwin.cpp,v
> retrieving revision 1.2
> diff -u -w -r1.2 chelixwindowwin.cpp
> --- chelixwindowwin.cpp 24 Oct 2002 20:50:48 -0000 1.2
> +++ chelixwindowwin.cpp 27 Sep 2003 11:25:05 -0000
> @@ -191,7 +191,7 @@
> 
> m_hWnd = CreateWindow(kWndClass,
> pTitle,
> -         WS_OVERLAPPEDWINDOW|WS_VISIBLE,
> +         WS_THICKFRAME | WS_SYSMENU|WS_OVERLAPPED|WS_CAPTION | WS_MINIMIZEBOX | \
> WS_VISIBLE, x,
> y,
> 0, // ignore width & height-- force us to
> 
> [Sunil] To disable the maximize button. Without this, when we press the maximize \
> button,  the application minimizes the window
> 
> 
> @@ -720,6 +720,7 @@
> SWP_SHOWWINDOW);
> 
> m_bInLayout = FALSE;
> +    RedrawWindow (m_hWnd, NULL, NULL, RDW_ERASE | RDW_INVALIDATE);
> }
> 
> [Sunil] It has been added in the Layout function to refresh the player window.
> 
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@video.helixcommunity.org
For additional commands, e-mail: dev-help@video.helixcommunity.org


[prev in list] [next in list] [prev in thread] [next in thread]