Odyssey's MediaPlayer thread

@kas1e

Else if branch in that function will never be called because EmptyPointer is NULL.

@Capehill
Right, should be out of function. Want to make it like this taking Mplayer's code:

@kas1e

videoenterfullpage method only called to enter/exit from full mode, not executed while playing video, this code should be executed inside VideBlit mode.

@Ami603
Indeed! Was in curious right now why have it called only one time and got it :)

@kas1e

BTW i've managed to get a laptop to compile odyssey and need to know how to enable the debug builds.

@Ami603
If you mean how enable debug output to serial then use D(bug("aasasasa")); bug() defined in Base/debug.h, and for os4 its:

Quote:

Also be sure that on top of file you not have #define D(x) which mean no debug, and it should be #define D(x) x. So D(bug("..")); will be taken in account.

If you mean how to made numbers of string be visibly in stack trace when crashes, then you need to add -gstabs to the objects you want to compile with. Better to add it only to the object you need , and not to whole compilation process. Or at least just for wholw webkit-owb part (where amiga native code placed), because if you will add -gstabs for whole WebCore, you will end up with 2 gb binary which you just can't even load on os4 to memory :)

@kas1e

done that, all i get is
[/amiga/Odyssey/odyssey-r155188-1.23/BAL/Types/WTF/MorphOS/BCMainThreadMorphOS.cpp:55 scheduleDispatchFunctionsOnMainThread] scheduleDispatchFunctionsOnMainThread()
kind of messages.

Nevermind, found it.
I was activating it for some unneeded file.

@All
Another question about the hiding of a cursor:

1). While we in full-window, when we hide mouse cursor: when cursor placed over the video area only or does not matter where cursor is (be it navigation menu, out of the window somewhere on a workbench, etc), if full window plays, then we in any case hide cursor after 2 seconds?

2). Do we need to go back to visibly cursor when we just move the mouse, or when we not only move the mouse, but also press mouse buttons?

3). Do we need to hide the mediaplayer control panel as well when hiding the cursor (like we press middle-mouse button), or keep it as it, and only hide mouse cursor?

@kas1e
Quote:
Here is my opinion.

1. Hide curser after 2 seconds regardless of location.

2. Bring back anytime mouse is moved.

3. Leaving controls visable is fine with me but I'm sure there are people who won't want that. If you do hide controls, have them come back on any mouse movement, same as cursor.

Thanks again for the work!

@ktadd
Quote:

I also like it more when i have gui of player be shown when i in full window. But from another side, when i hit "f11" (meaning i want to sit back and chill), then i definately want to hide gui and need some extra "middle mouse button" press for.

Probabaly it easer to leave it as it, and only hide mouse cursor. At least no one will be annoyed with thing he may don't like, and if want still can hide it via middle-mouse-button.

@kas1e

Quote:

Probably make it visible at mouse click (left or right click) will be better
Like MPlayer

@samo79Quote:
For me both of the latest MPlayer releases and Emotion make the pointer come back on mouse movement. No need to click. I personally like this better so the user doesn't have to guess how to make the pointer come back or wonder where it is.

@samo
Question is do we only unhide when mouse move, or and when mouse move & when mouse click.

@ktadd
I can made just and mouse move, and mouse click (through, mouse click without mouse move most of time unpossible) :) But formuself after few hours of tests, i find that pure mouse-move are more than enough.

@ktadd

Quote:

Yes, tested now the demo and indeed with Emotion the pointer will unhide at mouse movements, but with MPlayer it's not always like that

Atleast using p96 driver that i mostly use the behaviour is somekind different:

With MUI MPlayer (p96pip) left/right click or just move the mouse on fullscreen do nothing!

But with latest Mickjt MPlayer (p96pip) mouse unhide at clicks only, left or right buttons

@kas1e

Quote:

Well eventually you can implement it in both method :)
I mean, unhide it just when you move the mouse ... but also if you just left/right clicking without moving the mouse

Consider that right click should unhide the pointer in a mandatory way, even because it must open the context menu .. so rmb should unhide the pointer for sure

@kas1e

Unhide it when mouse moves since if you touch the mouse, it will anyway move (who manages to click without moving the mouse?).

@All

Added hiding of mouse cursor when we for 2 seconds do nothing, and then once mouse moved or mouse button clicked, then cursor showups again.

I do that hiding regardless of cursor position, just if we in fullscreen video playing, then hide the cursor after 2 seconds of inactivity. Through i not sure if that the right way now. It feels more logical (imho) to hide cursor when it only over the video area, but for next beta we can test if the way to hide mouse regardless position is way to go or not.

That how i do it (All changes in the owbbrowserclass.cpp):



So far, all works fine and as expected, except one thing: When I press "RMB" (so to have RMB menu), and start to navigate inside of the menu , the cursor also disappear, like, I didn't move the mouse. So I want to add a check like "if we in any MUI menu, do not start our time count". It should be surely in the OWBBrowser_VideoBlit , just instead of " if (!mouse_hidden) { } ", something like " if (!mouse_hidden && !_IF_NOT_RMB_MENU_OPENED_) { } ". The question there for me how to check if the RMB menu opened or not.

I can commit that what i have now, and maybe someone can revesit/check it all ?

@kas1e

Seems a coherent implementation, just this latest problem in menu and then probably it will be perfect :)

Do you think, later it will worth to implement also a screenblanker disactivation when in fullscreen ?
Probably it will be similar to what you did in dosbox

@kas1e

Some mostly stylistic comments:

1) what is the meaning of p_ ? Personally I would use whole words like "seconds" or "micros(econds)" to avoid confusion when it comes to variables.

2) This being C++, I would also use native bool/true/false for generic booleans, instead of BOOL/ULONG and their mix.

3) When you FreeVec EmptyPointer, it may be a good idea to set EmptyPointer to NULL, to avoid double-free.

4) Also, in C++ one doesn't have to say "struct" when using struct types. In other words, one can just say "Window" instead of "struct Window".

5) EmptyPointer could be created only once, when needed for the first time, and free'd somewhere where this object gets destroyed / cleanup.

Ps. At work I have to read a lot of code daily so sometimes this review role "sticks" :)

@Capehill
Quote:

Thanks ! Your review is one of those ones i was in hope for , so will clean it all like you say and commit in hope we can then got how to add check on RMB window opening, so to not have time-counter working when we in menu.

@Capehill

https://github.com/kas1e/Odyssey/commi ... 4035dfaa9079b262e9913c2c3

Strill trying to figure out why when i open RMB menu and move by mouse while it sticked , i didn't catch IDCMP_MOUSEMOVE events. Or we need to handle some other event, or we need to add to if (!mouse_hidden) part in videoblit , something else which will mean "and not RMB opened menu"

@Samo
Quote:

Seeing the code, there were already some morphos code for blanker deactivation/activation, and yeah, it happens only when it should happens : in fullscreen accelerated mode. Surely some ppls may not like that only because they run browser their screenblankers deactivated.

See : https://github.com/kas1e/Odyssey/blob/ ... pi/MorphOS/utils.cpp#L954

That the part we need to add for os4, as currently it just doing nothing. And code which use that enable_blanker() to enalbe/disable it called from OWBBrowser_VideoEnterFullPage method (i.e. when we go to fullscreen mode).

In other words, that probabaly need to be implemented, yes, but for you as for one who not have accelerated video playback it will make no differences.

Not sure if will be enough to just put there the same kind of code we use via application library when deactivate/activate screen blanker in dosbox.