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

[Reimar Döffinger]

On 6 Feb 2013, at 21:25, Mike Gorchak <mike.gorchak.qnx at gmail.com> wrote:
>> Uh, you are making my argument. The why is so that "it can report
>> playback status". It is not because you dislike mmap.
>> I still think that your comment is the equivalent of
>> a = a + 1; /* increment a by 1 */
>> It just repeats what the code does, but not what for example would
>> break if you removed it.
> 
> 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.

>>> You have read some old documentation. New says:
>>> "Returns: A positive value that represents the number of bytes that
>>> were successfully written to the device if the playback was
>>> successful. A value less than the write request size is an indication
>>> of an error; for more information, check the errno value and call
>>> snd_pcm_plugin_status()."
>> 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.

>>> I planned to use functions which will work with this struct. I've to
>>> fill this struct using multiple layers instead of one single layer.
>> I'm afraid I don't understand.
>> Neither needs a typedef, the latter does not even give the struct a
>> name.
> 
> 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.

>>>>>                  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.

>>> Nothing strange, window size has different dimension than drawable
>>> area. And not the screen size, but workspace size. Workspace size
>>> usually smaller than screen size.
>> Let me be specific:
>> Screen resolution 1024x768.
>> When playing a video with width 1024, this code will result in vo_dwidth
>> = 1024.
>> When playing a video with width 1026, this code will result in vo_dwitdh
>> = 1012
>> To summarize: A larger video will be played in a smaller window.
>> And that's not strange?
> 
> Screen size 1024x768
> Workspace size 860x720
> 
> 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.
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?