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

[Reimar Döffinger]

On Tue, Feb 05, 2013 at 10:32:32PM +0200, Mike Gorchak wrote:
> > 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.

> >>     /* Disable mmap to control data transfer to the audio device */
> >>     snd_pcm_plugin_set_disable(audio_handle, PLUGIN_DISABLE_MMAP);
> > The comment kind of explains the wrong thing, that it disables MMAP
> > is quite obvious, the interesting question the comment should answer is
> > _why_.
> 
> It is really answering on "why?" question. When you are using MMAP
> plugin, data transfer is controlled by audio driver, i.e. it can
> report playback status on sample basis, but can on fragment or even
> buffer basis. So you'll never know how many samples were played.

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.

> >>         if ((errno==EINVAL) || (errno==EIO))
> >
> > Yikes! Without checking written for an error you can't know
> > whether errno was set by this function or some completely unrelated
> > code e.g. in the demuxer.
> > More importantly, the way I read the documentation none of these
> > functions ever set errno, but "written" will contain -EINVAL
> > or -EIO.
> 
> 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?

> >> typedef struct _layer_mp_map
> >> {
> >> } layer_mp_map_t;
> >>
> >> layer_mp_map_t photon_layer_map[]=
> >
> > The typedef seems pointless, it is used only once.
> > You don't even have a reason to name the struct.
> 
> 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.
Both of these should work with the code you have:
struct layer_mp_map {
....
} static photon_layer_map[] =

struct {
....
} static photon_layer_map[] =

Neither needs a typedef, the latter does not even give the struct a
name.

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

> >>     if (vo_dwidth>(workspace_rect.lr.x-workspace_rect.ul.x))
> >>     {
> >>         vo_dx=workspace_rect.ul.x;
> >>         vo_dy=workspace_rect.ul.y;
> >>         vo_dwidth=(workspace_rect.lr.x-workspace_rect.ul.x)-12;
> >
> > Why are you resetting vo_dy here?
> 
> geometry() function places vo_dx and vo_dy to beyond of the screen, a
> window became uncontrollable.

But if bad vo_dx and vo_dy values cause issues then you have to
validate them specifically.
Resetting them if and only if vo_dwidth is too large is not the
right thing to do.
As you will probably notice when you try specifying some invalid
values with -geometry

> > And why the -12 in the assignment but not the if?
> 
> Sorry for magic number, I will move to #define declaration.
> 
> > That will cause strange behaviour as in a window that is exactly
> > screen size will have that size, while one one pixel larger will
> > actually become 12 pixels smaller.
> 
> 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?

> > However I can't really see how this could work, it never updates
> > vo_border??
> 
> I thought, It is set externally by mplayer's commands. Documentation
> libvo.txt says "VOCTRL_BORDER Makes the player window borderless." and
> nothing more. So should it work like vo_fs ?

Yes, those all should behave the same way.
Though easiest way to find out would be to just test the feature if it
actually works...

> > Yet another step beyond that would be suggestions for performance
> > improvements, like supporting slice rendering or direct rendering,
> > but that certainly is optional for a first implementation.
> 
> Direct rendering works, but it is much slower than indirect rendering.
> QNX doesn't support write combining cache settings for video memory
> regions, so direct video memory writes are usually slow. It is better
> to render an image to system ram and then perform fast_memcpy() to
> transfer its content to video memory.

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.