[MPlayer-dev-eng] [PATCH] AC3/DTS passthough supportfor MacOSX Intel

[? ?]

>From: Reimar Döffinger<Reimar.Doeffinger at stud.uni-karlsruhe.de>
>Reply-To: mplayer-dev-eng at mplayerhq.hu
>To: mplayer-dev-eng at mplayerhq.hu
>Subject: Re: [MPlayer-dev-eng] [PATCH] AC3/DTS passthough 
>supportfor	MacOSXIntel
>Date: Thu, 9 Aug 2007 10:31:36 +0200
>
> > > 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.
ok, removed.

>
>
> > +#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.
yes, it's not beautiful, this code is just directly from vlc's sources, as 
you said, it need be fixed.

>
> > +  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).
now aligned.

>
> >  	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!
if not return true, mplayer will make up a soft volume filter, it corrupted 
ac3/dts data whatever how much the volume setted, so here the only way work 
with passthrough is to return true.
indeed, when mac osx work in digital passthrough mode, the volume is 100% 
fixed, but the get property function used next the code will not work in 
such situation, so return true and 100% is correct answer here.

>
> > @@ -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.
yes, just added a warning message, it make sense because original code does 
not check the error info, print it into warning message will help debuger.
can't these be added in one 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.
as above explained, return true to avoid a soft volume filter when digital 
output.

>
> > @@ -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.
fixed, and merged with earlier stream format macro.

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

the main change in init(), is to identity whether current default audio 
output device support digital output, and if input data is ac3/dts encoded 
format, it init audio in function OpenSPDIF(). else, still the old audio 
unit init code there be used.

the reason why so many code be added to support ac3/dts passthrough is 
because the limit of mac osx audio unit framework (a higher level framework 
over audio hal framework) , it only support lpcm format input, to use 
ac3/dts passthrough, has to use the lower level audio hal framework,  
indeed, audio unit do lots work to simple codes for audio apps, but it just 
not support passthrough on current version of mac osx. by now at least in 
vlc, there are both audio unit and audio hal branches code for work with 
normal audio output and digital output.

since at least one year ago, vlc supported passthrough on mac osx, the 
source code was just there. as the most popular player on *nix system, 
mplayer should not lack this feature so long, it's time to support 
passthrough for mac users.

format fixed patch also posted here

>
>Greetings,
>Reimar Döffinger
>_______________________________________________
>MPlayer-dev-eng mailing list
>MPlayer-dev-eng at mplayerhq.hu
>http://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng

-------------- next part --------------
A non-text attachment was scrubbed...
Name: macosx_passthough_formatfix.patch
Type: application/octet-stream
Size: 39850 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20070809/251dd63c/attachment.obj>