[MPlayer-dev-eng] [PATCH] Support for QNX: QSA audio and Photon GUI.

[Mike Gorchak]

>> I think sometimes better to remove all comments from code.
> Which also would mean that nobody but you can maintain the code efficiently.
> The information that "if you remove this line, the get_delay function will break even though it looks completely unrelated" is indeed useful and worthy of a comment IMHO.

I disagree, it is unrelated for newbie but for the person who have
read the documentation at least once it is clear what to do:
http://developer.blackberry.com/native/documentation/bb10/com.qnx.doc.neutrino.audio/topic/libs/snd_pcm_channel_status_t.html

The note is highlighted with light blue color.

>>> Hm, maybe I just found another outdated one, but why the check for
>>> EINVAL?
>>> The documentation I found says for that:
>>>> Partial block buffering is disabled, but the size isn't the full block size.
>>> That doesn't sound like a case where you should be doing the
>>> reset/whatever code that is below this if?
>> It is the case, because EINVAL can be returned in some reasons.
>> Generally QSA uses virtual mixer which translates user sample formats
>> to the real hardware formats, and at some states it can return EINVAL.
>> At this moment playback could be stopped and must be reseted. I will
>> not remove this check.
>>
>> FYI: Here is the latest documentation:
>> http://developer.blackberry.com/native/documentation/bb10/com.qnx.doc.neutrino.audio/topic/libs/snd_pcm_plugin_write.html
>>
>> Please understand me, I'd prefer to not commit the code at all which
>> has specially included issue, just because you dislike EINVAL check.
> I don't want you to break things on purpose.
> But this code seems not possible to understand based on the documentation, it does not say that EINVAL can be returned for no good reason and to fix it a reset is needed.
> Code that handles behaviour that contradicts the documentation is another case worthy of a comment IMHO.

It is just a typical behaviour example: first time you will get
EINVAL, you return amount of written bytes, all bytes in the buffer
was already played and QSA raises an UNDERRUN condition. User at this
moment listen silence. Next time you try to write some bytes and get
an EIO error, after that you must perform re-preparation of playback
process and return zero written bytes. And only after third call user
will get sound playback. The sound will be a bit choppy. No one needs
a choppy sound.

So it is common practice on any error to ask about a current playback
status and this behaviour doesn't contradict with documentation. Got
an error - check the current state.

>> Is it a show stopper problem?
> No, but it would make the code smaller and avoid the _t issue without any real cost.
> I'd volunteer to make such a change before committing myself but
> a) I wouldn't be able to test it
> b) I don't know if it would end up with you being even more annoyed with me.

I would understand if my code somehow stands out from the rest of the
code. _t also is not an issue because it has prefixes which
distinguish user's type name from a standard one. BTW, QNX has a lot
of _t typedefs in the many utilities, libraries, applications, etc, so
it is some kind of OS style, nothing more.

>>>>>>                  PhPointerEvent_t* ptrevent=NULL;
>>>>>>                  ptrevent=PhGetData(info->event);
>>>>>
>>>>> Simpler as
>>>>> PhPointerEvent_t *ptrevent = PhGetData(info->event);
>>>> It is also a habit. Never dereference the pointer in the variable
>>>> declaration in assignment. For example, when info is NULL you will get
>>>> funny gdb backtrace.
>>> What? The compiler will create exactly the same code.
>>> If not it's a seriously strange/buggy compiler.
>>> Either way I have a hard time believing that such a workaround
>>> is worth obfuscating the code.
>> Why you think it is code obfuscation?
> Because you have an assignment that has neither effect nor purpose, and thus is misleading.
> For me that's kind of the definition of obfuscation.

It has one additional purpouse. If I insert something between pointer
declaration and PhGetData() which works with this pointer I will get a
segmentation fault, but in case of plain declaration without NULL
assignment I could get a normal access to some memory, which is not
desired.

>> Not the screen size, it is a workspace size! 12 pixels is a window
>> decoration size in pixels from the left and right sides. (I will add
>> this constant obtain dynamically a bit later). This is a screenshot of
>> ancient QNX version, but it is enough to get some info for you:
>> http://www.jfedor.org/shots/qnx.png . As you can notice all windows
>> are created inside a workspace, while taskbar at the bottom and launch
>> pad at the right side is not overlapped by windows. This code is
>> trying to fit window back to the workspace, so it shrinked more than
>> 12 pixels!
>
> screen, workspace, whatever it is called is not the point.
> The point is that your if creates a non-monotonic function.
> Just replace my 1024 with 860 etc. in my example.

Have you ever run mplayer on windows machine with
SDL/directx/direct3d/gl backend when display is tiny like 1024x600 or
1280x800 but you are trying to play 1920x1080 full hd video? Mplayer
has very ugly default behaviour - you can't drag the window, you can't
shrink it. The only one possibility to drag it - to press Ctrl-Space
and move the window manually using window menu.

In a QNX case by default mplayer window is placed in other workspaces,
which are neighbours of the current one, which is undesirable
behaviour. With my fixes all looks very naturally, all is really fine.

> Another way to express it: when checking if it fits into the workspace (condition inside the if) you don't take the decoration into account, only when calculating the new size.
> Or another way to say it: shouldn't the -12 be inside the if() as well?

Yes, it must be, but in another form. I've added this and another
fixes in a new patch, which I've attached.

Thanks in advance!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ao_qsa.c
Type: text/x-csrc
Size: 15061 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20130207/d480de6f/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer-qnx-aovo3.diff
Type: application/octet-stream
Size: 76455 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20130207/d480de6f/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vo_photon.c
Type: text/x-csrc
Size: 49752 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20130207/d480de6f/attachment-0003.bin>