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

[Mike Gorchak]

Hi Reimar,

>> snd_pcm_channel_params_t cpars;
>> snd_pcm_channel_setup_t  csetup;
>> snd_pcm_t* audio_handle=NULL;
>> int audio_fd;
> These should be all static.

I agree. It is in my todo list.

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

> You could merge the identical cases.

I agree.

> In addition
> frag_multiplier = af_fmt2bits(ao_data.format) / 8;
> seems to be a simpler way to do the same thing.

I agree.

>>         default:
>>              qsa_format=SND_PCM_SFMT_S16_LE;
>>              frag_multiplier=2;
>
> Shouldn't you assign "format" as well here?

Yes, you are right.

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

>>     /* Check if QSA and underlying driver can accept requested parameters */
> So I don't think this is going to work particularly well,
> and for not working all that great it is a lot of code.
> A simple, small list of fixed fallback configurations
> seems much more likely to me to work, plus simpler to understand.

I agree, this is already in my todo list.

>> static void uninit(int immed)
>> {
>>     snd_pcm_playback_flush(audio_handle);
>>     snd_pcm_close(audio_handle);
>
> This is wrong, you should only wait for all output to be played
> if immed is 0.

Ok, I will fix this.

>> static void drain(void)
>> {
>>     snd_pcm_plugin_playback_drain(audio_handle);
>> }
> Hm, maybe I'm blind but this doesn't seem to be used.
> Doesn't the compiler warn about that?

Thanks, but It is already deleted :)

>> static int play(void* data, int len, int flags)
>> {
>>     int written;
>>     int status;
>>     snd_pcm_channel_status_t cstatus;
>>
>>     written=snd_pcm_plugin_write(audio_handle, data, len);
>
> Other aos round len to outburst, to avoid bad things from happening
> (like channels becoming swapped, this is especially a risk for
> 5 channel audio).
> I can't know for sure if it's necessary here, maybe snd_pcm_plugin_write
> does something similar on its own.

snd_pcm_plugin_write takes care about hardware buffer filling, it also
translates logical frags to hardware frags. It also translates
unsupported by hardware sample formats/rates/voices to supported. But
I agree I have to setup some reasonable outburst value.

>>     if (written!=len)
>>     {
>>         /* Check if samples playback got stuck somewhere in hardware or in */
>>         /* the audio device driver */
>>         if ((errno==EAGAIN) && (written==0))
>>         {
>>             return 0;
>>         }
>
> What's the point of that? You'll return 0 just as much
> by just removing this code.

Ok.

>>         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()."


>>             status=snd_pcm_plugin_status(audio_handle, &cstatus);
>>             if (status>0)
>
> According to the documentation this makes no sense, since
> status will always be <= 0.

Yes, you are right, this code is a workaround for very old QSA versions.

>> PdOffscreenContext_t* ph_image_off=NULL;
>> PdOffscreenContext_t* ph_image_off2=NULL;
>> void*                 ph_surface_data=NULL;
>> void*                 ph_surface_data2=NULL;
>
> Why aren't these simply arrays? This would allow simplifying a lot of
> code. Also, explicitly writing =0 or = NULL is completely unnecessary.

I've planned to re-organize the global variables soon.

>> int                   ph_layer_contrast=0;
>> int                   ph_layer_saturation=0;
>> int                   ph_layer_hue=0;
>
> These all need to "static" to no risk symbol collisions.

I agree.

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

> Also, it should be static to not pollute the global symbols.

Ok.

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

> Also, while a very minor thing, generally FFmpeg/MPlayer style
> is to have the * attached to the variable name, not the type.
> It also would be simpler to have a
> if (!vo_nomouse_input)
>   break;
> at the start instead of wrapping each mplayer_put_key separately
> into an if.

Already fixed.

>>     if (ph_image_off!=NULL)
>
> Minor thing: The "!=NULL" is completely unnecessary and writing
> just
> if (ph_image_off)
> is more in line with the rest of the code.

It is just a different codestyle, nothing more.

>>     aspect_save_orig(width, height);
>>     aspect_save_prescale(d_width, d_height);
>>     aspect_save_screenres(vo_screenwidth, vo_screenheight);
>>     geometry(&vo_dx, &vo_dy, &vo_dwidth, &vo_dheight, vo_screenwidth, vo_screenheight);
>
> For vos using the new UPDATE_SCREENINFO support should only
> call aspect_save_screenres from the code handling that and
> it should never call any of the other functions.
> vo_dx/vo_dy/vo_dwidth/vo_dheight should all be set up correctly
> by the time this code is reached.

Ok, I will remove this.

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

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

>>     if ((flags & VOFLAG_FULLSCREEN)==VOFLAG_FULLSCREEN)
>>     {
>>          vo_fs=VO_TRUE;
>
> This I believe is inconsistent with other code, even though it
> _currently_ will compile to the same thing.
> if (flags & VOFLAG_FULLSCREEN)
> {
>   vo_fs = 1;

Ok, I will change this.

>>     /* Try to use an offscreen renderer */
>>     if (phrender_type==PHRENDER_USE_NONE)
>>     {
>>         ph_image_off=PdCreateOffscreenContext(photon_display_format, width, height,
>>             Pg_OSC_MEM_2D_WRITABLE | Pg_OSC_MEM_2D_READABLE | Pg_OSC_MEM_PAGE_ALIGN);
>>         if (ph_image_off!=NULL)
>>         {
>>             phrender_type=PHRENDER_USE_OFFSCREEN;
>>             ph_surface_data=PdGetOffscreenContextPtr(ph_image_off);
>>             ph_surface_width=ph_image_off->dim.w;
>>             ph_surface_height=ph_image_off->dim.h;
>>             ph_surface_pitch=ph_image_off->pitch;
>>         }
>>     }
>>
>>     /* Try to use an software offscren renderer */
>>     if (phrender_type==PHRENDER_USE_NONE)
>>     {
>>         ph_image_off=PdCreateOffscreenContext(photon_display_format, width, height,
>>             Pg_OSC_MEM_PAGE_ALIGN | Pg_OSC_MEM_SYS_ONLY |
>>             Pg_OSC_MEM_HINT_CPU_READ | Pg_OSC_MEM_HINT_CPU_WRITE);
>>         if (ph_image_off!=NULL)
>>         {
>>             phrender_type=PHRENDER_USE_SWOFFSCREEN;
>>             ph_surface_data=PdGetOffscreenContextPtr(ph_image_off);
>>             ph_surface_width=ph_image_off->dim.w;
>>             ph_surface_height=ph_image_off->dim.h;
>>             ph_surface_pitch=ph_image_off->pitch;
>>         }
>>     }
>
> That's basically the same code twice. A function or similar for that
> would be good.

Three lines of duplicated code is not a big deal.

>>         case VOCTRL_BORDER:
>>              if (vo_border)
>>              {
>>              }
>>              return VO_TRUE;
>
> Much simpler as something like
> case VOCTRL_BORDER:
>   if (!phwindow)
>     break;
>   PtSetResource(phwindow, Pt_ARG_WINDOW_RENDER_FLAGS, vo_border ?  Pt_TRUE : Pt_FALSE, Ph_WM_RENDER_BORDER);
>   PtFlush();
>   return VO_TRUE;

In future I planned to add different resources programming, since
borderless window can't be resized in common way, so resizing must be
disabled.

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

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