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

[Mike Gorchak]

>> > In addition, there is no point in initializing to NULL explicitly, it is
>> > implicit.
>> It's a habit, when using different gcc compilers for different
>> platforms you never know where your unitialized global variable will
>> be in .bss or .data segment. So when you initialize it, it will go to
>> .bss section definitely.
>
> That sounds like extraordinarily stupid behaviour, putting it
> in .data means wasting disk space for storing 0s for no good reason.

This is how it works. I'm not planning to argue with you on such minor
behaviour, but take a look on numerous gcc forks for different
embedded CPUs, they usually uses ancient versions of gcc or tweaked
gcc because of their memory layout, etc, etc. I do not like surprises,
so I initialize all variables which will be used before assignment in
the code.

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

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

>> >>                   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? It is a common practice when you
are planning to add checks or trace output later. If I will need to
insert some debug output I will not rewrite the code.

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

> That reminds me that you might want to look into mem2agpcpy instead
> of fast_memcpy, it can be significantly faster when copying to video
> memory.

I will try, thanks.