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

[Ed Wildgoose]

Thanks for the comments, new patch attached, and thoughts inline below:


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

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

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

>>      unsigned int bits_per_sample;
>> -   
>> +
>
> cosmetics.


Yeah, sorry about that.  Something to do with my copy of vi 
interchanging tabs and spaces.  I have never quick mastered the relevant 
config to control this properly...  Will resubmit


>> +    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)


> You only removed a lonely tab in this last line...


Yeah, sorry.  Does it matter?


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


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


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

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


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


New patch attached

Thanks

Ed W


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: mplayer_jack3.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20041018/694f1ed6/attachment.txt>