[FFmpeg-devel] [PATCH] libavdevice: JACK demuxer

[Michael Niedermayer]

On Mon, Mar 02, 2009 at 03:18:51PM +0100, Olivier Guilyardi wrote:
> Attached:
> - jack demuxer patch version 0.7
> - time filter patch version 0.1
> 
> The later provides DLL time filtering as a libavformat patch (I'm not sure about
> the Makefile changes).
> 
> Michael Niedermayer wrote:
[...]
> > the 2 parameters that control the filter are locked together using a
> > formular using sqrt() PI and others, while iam sure this can be proofen
> > to be optimal in a idealized case its certainly not in all practical
> > cases, the user should have the chance to set both independantly, and
> > as a sideeffect the dependancy on math.h can then be droped
> 
> The parameters of the filter are a complex topic, which users shouldn't play
> with IMO. Please fully read the pdf document mentionned earlier.

ive read over it mostly now, there is no hint of why the parameters are locked
together.
And about users, i did not mean the end user should mess with the parameters
but the optimal values clearly depend on the hardware and there are 2 not 1
from which 2 can be found. It surely would make sense to be able to set them
based on the kind of audio hw there is which doesnt mean that has to be done
just that writing the code so it cannot be done is nasty

Besides strictly speaking the optimal values of these parameters are non
constant. To see why just consider that the exact samplingrate is initially
not known exactly and has to be estimated first, after that, given reasonable
quality hardware it is not going to change by a large amount IMHO.

also the authors of the paper test the filter meassuring its jitter, i dont
see in how far this is meassuring the quality of the filter, its easy to apply
heavy enough filtering to reduce the jitter between timestamps to whatever
value one desires this
though does not make the timestamps actually represent reality. Just consider
that the jitter was truly due to inaccurate sampling times in the hardware.

It seems the easiest way to test such filter would be to simulate timestamps
+ random noise sampled with a slowly drifting samplerate (simulating the
effects of heat on the clock generator)
With such simulation one could then test how close the filter can recover the
true sampling times, because all things are exactly known in such a simulation
this is harder with actual hardware where one doesnt know the true sampling
times, but could probably done too given some high quality source signal


[...]

> +/*
> + * Size of the internal ringbuffer as a number of jack buffers
> + * Must be greater than or equal to 2
> + */
> +#define RING_NBUFFERS 4

that isnt doxygen compatible


[...]
> +#ifdef WORDS_BIGENDIAN
> +    stream->codec->codec_id = CODEC_ID_PCM_F32BE;
> +#else
> +    stream->codec->codec_id = CODEC_ID_PCM_F32LE;
> +#endif

I suspect we should add a CODEC_ID_PCM_F32NE that is #defined to
one of above to get rid of such #ifdefs in several places ...


> +    stream->codec->sample_rate = self->sample_rate;
> +    stream->codec->channels = self->nports;
> +
> +    av_set_pts_info(stream, 64, 1, 1000000);  /* 64 bits pts in us */
> +    return 0;
> +}
> +
> +static int audio_read_packet(AVFormatContext *context, AVPacket *pkt)
> +{
> +    PrivateData *self = context->priv_data;
> +    int pkt_size;
> +    struct timespec timeout = {0, 0};
> +

> +    if (!self->activated && activate_jack(self, context))
> +        return AVERROR(EIO);

this looks like it belongs in audio_read_header()



> +
> +    timeout.tv_sec = av_gettime() / 1000000 + 2;
> +    if (sem_timedwait(&self->packet_signal, &timeout)) {
> +        if (errno == ETIMEDOUT) {
> +            av_log(context, AV_LOG_ERROR,
> +                   "Input error: timed out when waiting for JACK process callback output\n");
> +        } else {
> +            av_log(context, AV_LOG_ERROR, "Error while waiting for audio packet: %s\n",
> +                   strerror(errno));
> +        }
> +        if (!self->client) {
> +            av_log(context, AV_LOG_ERROR, "Input error: JACK server is gone\n");
> +        }
> +        return AVERROR(EIO);
> +    }
> +
> +    if (self->overrun) {
> +        av_log(context, AV_LOG_WARNING, "Packet ringbuffer overrun\n");
> +        self->overrun = 0;
> +    }
> +
> +    if (self->xrun) {
> +        av_log(context, AV_LOG_WARNING, "JACK xrun\n");
> +        self->xrun = 0;
> +    }
> +

> +    self->reader_index = (self->reader_index + 1) % RING_NBUFFERS;
> +

> +    memcpy(pkt, self->ringbuffer + self->reader_index, sizeof(*pkt));

*pkt= self->ringbuffer[self->reader_index];

also it seems 1 entry in the ring buffer cant be used with your
implementation

maybe the following is better:
process_callback(){
    if((uint8_t)(writer_index - reader_index) < RING_SIZE){
        write at buffer[writer_index & (RING_SIZE-1)];
        writer_index++;
    }
}

read_packet(){
    if(writer_index == reader_index)
        return AVERROR(EAGAIN);

    take packet from buffer[reader_index & (RING_SIZE-1)];
    buffer[reader_index & (RING_SIZE-1)]= av_new_packet();
    reader_index++;
}

note, both writer_index and reader_index must be unsigned for above
to work, and probably its best if they are uint8_t to exclude any issues
with the writes to them not being done in one piece.


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090303/327236f4/attachment.pgp>