[MPlayer-dev-eng] Patch: Jack audio output

[Reimar Döffinger]

Hi,
> >>+long approx_bytes_in_jackd = 12288; // Rough estimate (=512 frames * 
> >>6 channels * 2 bytes * 2 buffers
> >
> >when is this default used? Does jack always use 6 channels? This seems 
> >fishy to me.
> 
> Sure, leave it set to zero then.  It gets assigned in the Init function 
> and also in the Reset function anyway.  I only had it set like this so 
> that when I was testing I could quickly comment out the other ways that 
> it was set and play with the value here. 
> 
> I will resubmit with it set to zero.  This is a red herring really.
> 
> >>-long JACK_Write(int deviceID, char *data, unsigned long bytes); /* 
> >>returns the number of bytes written */
> >>+long JACK_Write(int deviceID, unsigned char *data, unsigned long 
> >>bytes); /* returns the number of bytes written */
> >
> >This change is rather irrelevant. Leave it as it was I'd say
> 
> We can do, but the actual API changed in bio2jack to make it an unsigned 
> char...  For whatever reason we aren't loading the bio2jack.h file and 
> instead portions of it are reproduced here directly...

Don't know why, I wouldn't mind changing it, although people will have
to have it lie around somewhere like this...

> ...However, this raises a seperate issue which is that we are compiling 
> to use bio2jack as a shared library... For the sake of a small header 
> file and a block of code I would much prefer to simply include bio2jack 
> in mplayer source and then we can at least be sure that we are 
> distributing something which works to the end user... CVS would 
> obviously then have to track bio2jack source, but at least we don't 
> break when bio2jack breaks.  What do you think?  I can provide all the 
> files, but I'm not sure how to change the various makefiles...?

Now that there are bio2jack releases and not only CVS everyone can
downgrade to the last release if it breaks...
Having bio2jack directly included would require someone regularly
updating it - a waste of developer time at least I won't be happy with,
which is why it was already refused at the time of first commit.
And when I look at what a mess every update of libfaad or libmpeg2 seems
to cause I am even more against it.

> >> long JACK_GetJackLatency(int deviceID); /* return the latency in 
> >>milliseconds of jack */
> >> int  JACK_SetState(int deviceID, enum status_enum state); /* 
> >>playing, paused, stopped */
> >>-int  JACK_SetVolumeForChannel(int deviceID, unsigned int channel, 
> >>unsigned int volume);
> >>+int  JACK_SetAllVolume(int deviceID, unsigned int volume); /* 
> >>returns 0 on success */
> >>+int  JACK_SetVolumeForChannel(int deviceID, unsigned int channel, 
> >>unsigned int volume); /* return 0 on success */
> >
> >You only added a comment to the line with JACK_SetVolumeForChannel. If 
> >you really want those comments put them on seperate lines and make 
> >them doxygen-style.
> 
> Fair enough.  I think this was a cut and paste from bio2jack.h.  I will 
> remove the comments.
> 
> (I added comments to all lines though, so not sure exactly that this was 
> what you meant?)

I only dislike it when lines appear in the diff that haven't changed
functionally...

> >>+    unsigned int jack_port_name_count=0;
> >>+    const char *jack_port_name=NULL;
> >>+
> >>     mp_msg(MSGT_AO, MSGL_INFO, "AO: [Jack] Initialising library.\n");
> >>     JACK_Init();
> >> 
> >>+        if (ao_subdevice) {
> >>+        jack_port_flags = 0;
> >>+        jack_port_name_count = 1;
> >
> >What purpose does this variable have?
> 
> Which variable?  They are all used in Jack_OpenEx.  Basically the syntax 
> is slightly different if you just want it to pick a handy physical 
> output device, versus whether you want it to bind to some other filter 
> in the chain.  This code should handle those conditions. (without a 
> bunch of IF statements)

ok, I first thougth it was always 1...

> >You only removed a lonely tab in this last line...
> 
> Yeah, sorry.  Does it matter?

It's cosmetics ;-)

> >>+        // Rather rough way to find out the rough number of bytes 
> >>buffered
> >>+        approx_bytes_in_jackd = JACK_GetJackBufferedBytes(driver) * 2;
> >>+   
> >
> >Maybe a bit too rough a way... This might break more than it fixes...
> 
> Unlikely (to be worse than not doing it).  We are counting "backwards", 
> and the function call tells us the size of the audio buffer.  We know 
> that there must be at least two buffers, and also that Jack defaults to 
> using two buffers, and also that Jack will frequently be left with two 
> buffers for real use (since it's a low latency audio system).
> 
> If you take this line out then you will *always* have lipsync issues.  
> If you leave it in, then you *may* have lipsync issues in a few unusual 
> configurations.  However, right now there is no API to do better than 
> that, but I am talking with the bio2jack author about how we can do 
> better.  Expect a later patch to improve on this behaviour, but I think 
> at the moment it is pretty good as per above.

Ok for me, if it's only a workaround for missing API... Although I still find
it ugly :-(

> >> static int play(void* data,int len,int flags)
> >> {
> >>-    return JACK_Write(driver, data, len);
> >>+    int ret=0;
> >>+    ret = JACK_Write(driver, data, len);
> >>+    return ret;
> >> }
> >
> >I can see no functional change here...
> 
> Agreed.  It is only a little more convenient for testing.  Would you 
> prefer not to have this kind of thing changed then?

No functional change = cosmetics...
You can keep it for better testing in your local copy if you want,
but not for CVS.

> >>@@ -181,6 +213,9 @@
> >> static void reset()
> >> {
> >>     JACK_Reset(driver);
> >>+    latency = JACK_GetJackLatency(driver);
> >>+    // Rather rough way to find out the rough number of bytes buffered
> >>+    approx_bytes_in_jackd = JACK_GetJackBufferedBytes(driver);
> >
> >Why is this done here?
> 
> As stated above, we should perhaps check the size of the audio buffers 
> constantly, but since it's highly unusual (and quite difficult) to 
> change the size of the audio buffer whilst actually running then we can 
> compromise and just check the size of the buffer when we init the driver 
> and on any major resets.

Checking constantly is a bad idea IMHO, so it's probably best to leave
as-is.

> I don't actually know when Reset is called in general though?  Perhaps 
> it never is called in any useful situations?

It is used on seek, to throw away data from before the seek (mplayer.c,
line 3576)

> >>@@ -192,6 +227,8 @@
> >> 
> >> static float get_delay()
> >> {
> >>-    return (float )JACK_GetJackLatency(driver);
> >>+    float ret=0;
> >>+    ret = (JACK_GetBytesStored(driver) + approx_bytes_in_jackd + 
> >>latency) / (float)ao_data.bps;
> >>+    return ret;
> >> }
> >
> >Either this is wrong or the JACK_GetJackLatency function has an 
> >idiotic name I think...
> 
> Nope, it's correct and the name is not too idiotic.  The 
> JACK_GetJackLatency gets the fixed latency of the Jack audio server 
> (usually zero).  It is measured in terms of the number of buffers of 
> delay which will be incurred as a result of a complex graph, or some sub 
> filter which creates a filter delay.  On the other hand bio2jack has 
> it's own larger audio buffer (since we are converting a callback style 
> api to a blocking style api) and this is measured by the 
> JACK_GetBytesStored function
> 
> So our calculation here is:  "Bytes in the bio2jack layer" + bytes in 
> the jack subsystem (fixed) + overhead/latency in the jack graph
> 
> Does that make any sense?

It wouldmake much more sense if those "bytes in 
the jack subsystem" were included in the value from the
JACK_GetJackLatency...

>  	mp_msg(MSGT_AO, MSGL_INFO, "AO: [Jack] Initialising library.\n");
>  	JACK_Init();
>  
> +        if (ao_subdevice) {
> +		jack_port_flags = 0;
> +		jack_port_name_count = 1;
> +		jack_port_name = ao_subdevice;
> +		mp_msg(MSGT_AO, MSGL_INFO, "AO: [Jack] Trying to use jack device:%s.\n", ao_subdevice);
> +        }
> +

[...]
>  	
> +        err = JACK_SetAllVolume(driver, 100);
> +        if(err != ERR_SUCCESS) {
> +		// This is not fatal, but would be peculiar...
> +                mp_msg(MSGT_AO, MSGL_ERR,
> +                                "AO: [Jack] JACK_SetAllVolume() failed, error %d\n", err);
> +        }

[...]
> +        // Rather rough way to find out the rough number of bytes buffered
> +        approx_bytes_in_jackd = JACK_GetJackBufferedBytes(driver) * 2;
Here you use spaces instead of tabs for intendation... that makes the
code look bad with a different tabsize...

Greetings,
Reimar Döffinger