[MPlayer-dev-eng] [PATCH] AC3/DTS passthough support for MacOSXIntel

[Reimar Döffinger]

> > Add support for AC3/DTS passthrough to S/PDIF output on macosx according
> > codes from auhal.c in VLC's sources.
> >
> > also add support to restore digital output when re-plug s/pdif.
> >
> > tested on macbook intel osx 10.4.10, not tested on power pc based mac (I
> > think it need adapt ac_hwac3.c also for byte order fix)
> >
> > Note: Before use passthrough, you'd better reboot your mac system once with
> > s/pdif pluged to avoid strange audio not alive problem (after reboot it
> > works fine)


> + * 9/8-2007: Add support for AC3/DTS passthrough to S/PDIF output on macosx by ulion
> + *			 passthough need talk to hal, lots of codes are from auhal.c in VLC's sources.
> + *			 new add support to restore digital output when re-plug s/pdif.
> + *			 Before use passthrough, you'd better reboot your mac system once with s/pdif pluged to avoid
> + *			 strange audio not alive problem (after reboot it works fine)
> + *

This belongs in the commit message or general documentation, no need to have it in the source.


> +#define STREAM_FORMAT_MSG( pre, sfm ) \
> +    pre "[%ld][%4.4s][%ld][%ld][%ld][%ld][%ld][%ld]\n", \
> +    (UInt32)sfm.mSampleRate, (char *)&sfm.mFormatID, \
> +    sfm.mFormatFlags, sfm.mBytesPerPacket, \
> +    sfm.mFramesPerPacket, sfm.mBytesPerFrame, \
> +    sfm.mChannelsPerFrame, sfm.mBitsPerChannel

Using a function is a much less ugly way to do this. Also %ld is not the
right type for (UInt32). If you're pendantic, not even %d is and you
should use uint32_t and PRId32.

> +  AudioDeviceID i_selected_dev;				/* Keeps DeviceID of the selected device */
> +  int b_supports_digital;					/* Does the currently selected device support digital mode? */
> +  int b_digital;							/* Are we running in digital mode? */

You should align the comments (and avoid tabs, they always look messed
up for at least half the people looking at it).

>  	case AOCONTROL_GET_VOLUME:
>  		control_vol = (ao_control_vol_t*)arg;
> +		if(ao->b_digital)	// digital output can not adjust output volume
> +		{
> +			control_vol->left=control_vol->right=100.0;
> +			return CONTROL_TRUE;
> +		}

returning CONTROL_TRUE when volume control does not work certainly isn't
right!

> @@ -185,10 +219,13 @@
>  			return CONTROL_TRUE;
>  		}
> 		else {
> +			ao_msg(MSGT_AO, MSGL_WARN, "could not get HAL output volume: [%4.4s]\n", (char *)&err );
>  			return CONTROL_FALSE;
>  		}

Unrelated to AC3/SPDIF support? Belongs to a separate patch.

>  	case AOCONTROL_SET_VOLUME:
> +		if(ao->b_digital)	// digital output skip set volume
> +			return CONTROL_TRUE;

Same as above, CONTROL_FALSE is the right thing when volume control is
not available.

> @@ -198,6 +235,7 @@
>  			return CONTROL_TRUE;
>  		}
>  		else {
> +			ao_msg(MSGT_AO, MSGL_WARN, "could not set HAL output volume: [%4.4s]\n", (char *)&err );
>  			return CONTROL_FALSE;
>  		}

Unrelated?

> @@ -211,7 +249,7 @@
>  static void print_format(const char* str,AudioStreamBasicDescription *f){
>      uint32_t flags=(uint32_t) f->mFormatFlags;
>      ao_msg(MSGT_AO,MSGL_V, "%s %7.1fHz %dbit [%c%c%c%c] %s %s %s%s%s%s\n",
> -	    str, f->mSampleRate, f->mBitsPerChannel,
> +	    str, f->mSampleRate, (int)f->mBitsPerChannel,

Unrelated change as well.

> static int init(int rate,int channels,int format,int flags)

I can't really imagine that it is necessary to almost completely rewrite
this function to add AC3 support instead of changing just a few lines.
At least such a huge change is far beyond what I can review.

Greetings,
Reimar Döffinger