[MPlayer-dev-eng] [PATCH] Support for QNX: QSA audio and Photon GUI.
Reimar Döffinger
Reimar.Doeffinger at gmx.de
Tue Feb 5 20:19:51 CET 2013
On Tue, Feb 05, 2013 at 10:53:56AM +0200, Mike Gorchak wrote:
> 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.
In addition, there is no point in initializing to NULL explicitly, it is
implicit.
> case AF_FORMAT_AC3_LE:
> qsa_format=SND_PCM_SFMT_IEC958_SUBFRAME_LE;
> frag_multiplier=2;
> break;
> case AF_FORMAT_AC3_BE:
> qsa_format=SND_PCM_SFMT_IEC958_SUBFRAME_BE;
> frag_multiplier=2;
> break;
> case AF_FORMAT_IEC61937_LE:
> qsa_format=SND_PCM_SFMT_IEC958_SUBFRAME_LE;
> frag_multiplier=2;
> break;
> case AF_FORMAT_IEC61937_BE:
> qsa_format=SND_PCM_SFMT_IEC958_SUBFRAME_BE;
> frag_multiplier=2;
> break;
You could merge the identical cases.
In addition
frag_multiplier = af_fmt2bits(ao_data.format) / 8;
seems to be a simpler way to do the same thing.
> default:
> qsa_format=SND_PCM_SFMT_S16_LE;
> frag_multiplier=2;
Shouldn't you assign "format" as well here?
> /* 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_.
> /* Check if QSA and underlying driver can accept requested parameters */
> status=snd_pcm_plugin_params(audio_handle, &cpars);
> if (status<0)
> {
> do {
> if ((format==AF_FORMAT_FLOAT_LE) && (try_again==0))
> {
> format=AF_FORMAT_S32_LE;
> qsa_format=SND_PCM_SFMT_S32_LE;
> cpars.buf.block.frag_size=8192*4;
> try_again=1;
> }
> if ((format==AF_FORMAT_FLOAT_BE) && (try_again==0))
> {
> format=AF_FORMAT_S32_BE;
> qsa_format=SND_PCM_SFMT_S32_BE;
> cpars.buf.block.frag_size=8192*4;
> try_again=1;
> }
> if ((format==AF_FORMAT_S32_LE) && (try_again==0))
> {
> format=AF_FORMAT_S16_LE;
> qsa_format=SND_PCM_SFMT_S16_LE;
> cpars.buf.block.frag_size=8192*2;
> try_again=1;
> }
> if ((format==AF_FORMAT_S32_BE) && (try_again==0))
> {
> format=AF_FORMAT_S16_BE;
> qsa_format=SND_PCM_SFMT_S16_BE;
> cpars.buf.block.frag_size=8192*2;
> try_again=1;
> }
> if ((channels>=6) && (try_again==0))
> {
> channels=4;
> try_again=1;
> }
> if ((channels>=4) && (try_again==0))
> {
> channels=2;
> try_again=1;
> }
> if ((channels>=2) && (try_again==0))
> {
> channels=1;
> try_again=1;
> }
> if ((rate>=192000) && (try_again==0))
> {
> rate=96000;
> try_again=1;
> }
> if ((rate>=96000) && (try_again==0))
> {
> rate=48000;
> try_again=1;
> }
> if ((rate>=48000) && (try_again==0))
> {
> rate=44100;
> try_again=1;
> }
I'm not convinced of this code, it falls down to using only
1 channel before it even tries to reduce the sample rate.
It also will never try to switch from AF_FORMAT_S16_BE to
AF_FORMAT_S16_LE, which if it behaves in any way similar
to ALSA means it'll probably fail completely for AF_FORMAT_S16_BE
input.
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.
> 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.
> 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?
> 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.
> 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.
> 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.
> 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.
> int phwindow_fullscreen_set=VO_TRUE;
>
> int phinited=0;
> PtWidget_t* phwindow=NULL;
> PtWidget_t* phoscontainer=NULL;
> PtWidget_t* phrawcontainer=NULL;
>
> #define PHW_WINDOW_CHANGE_POSITION 0x00000001
> #define PHW_WINDOW_CHANGE_SIZE 0x00000002
> #define PHW_WINDOW_EXIT_REQUESTED 0x00000004
> #define PHW_WINDOW_MAXIMIZE_EVENT 0x00000008
>
> unsigned int phwindow_event_flags;
> PhDim_t phwindow_dimension;
> PhDim_t phwindow_old_dimension;
> PhPoint_t phwindow_position;
> PhPoint_t phwindow_old_position;
> PhDim_t phwindow_fs_dimension;
> PhPoint_t phwindow_fs_position;
>
> unsigned long photon_display_format;
> int photon_mplayer_format;
> int photon_format_idx=0;
> int photon_screen_width;
> int photon_screen_height;
> int photon_video_width;
> int photon_video_height;
> int photon_target_video_pos_x;
> int photon_target_video_pos_y;
> int photon_target_video_width;
> int photon_target_video_height;
> int photon_source_video_pos_x;
> int photon_source_video_pos_y;
> int photon_source_video_width;
> int photon_source_video_height;
>
> #define PHRENDER_USE_NONE -1
> #define PHRENDER_USE_LAYER 0
> #define PHRENDER_USE_OFFSCREEN 1
> #define PHRENDER_USE_SWOFFSCREEN 2
>
> int phrender_type=PHRENDER_USE_NONE;
> int ph_image_current=0;
> 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.
> int ph_surface_width;
> int ph_surface_height;
> int ph_surface_pitch;
> PhPoint_t ph_position;
> PhRect_t ph_src_rect;
> PhRect_t ph_dst_rect;
> unsigned long ph_chroma_color;
>
> /* Layer settings */
> int photon_layer_map_id=-1;
> int ph_layer_gamma=0;
> int ph_layer_brightness=0;
> 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.
> typedef struct _layer_mp_map
> {
> unsigned int layer_id;
> unsigned int mp_id;
> unsigned int format_idx;
> unsigned int found;
> unsigned int layer_idx;
> unsigned int layer_caps;
> unsigned int layer_chroma_caps;
> } 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.
Also, it should be static to not pollute the global symbols.
> typedef struct _photon_mp_keydef
> {
> int photon_key;
> int mp_key;
> } photon_mp_keydef_t;
>
> photon_mp_keydef_t photon_keymap[]=
Same as above, except it also should be "const".
> PhPointerEvent_t* ptrevent=NULL;
>
> ptrevent=PhGetData(info->event);
Simpler as
PhPointerEvent_t *ptrevent = PhGetData(info->event);
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.
> 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.
> 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.
> 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?
And why the -12 in the assignment but not the if?
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.
> 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;
would be in line with what other vos do.
> /* 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.
> case VOCTRL_BORDER:
> if (vo_border)
> {
> if (phwindow)
> {
> PtSetResource(phwindow, Pt_ARG_WINDOW_RENDER_FLAGS, Pt_TRUE,
> Ph_WM_RENDER_BORDER);
> PtFlush();
> }
> }
> else
> {
> if (phwindow)
> {
> PtSetResource(phwindow, Pt_ARG_WINDOW_RENDER_FLAGS, Pt_FALSE,
> Ph_WM_RENDER_BORDER);
> PtFlush();
> }
> }
> 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;
However I can't really see how this could work, it never updates
vo_border??
Note: this is only a very quick look through the vo code, I haven't
had the time to try and understand and review any of the actual logic.
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.
Regards,
Reimar Döffinger
More information about the MPlayer-dev-eng
mailing list