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

[Olivier Guilyardi]

Michael Niedermayer a ?crit :
> On Wed, Mar 04, 2009 at 02:33:47PM +0100, Olivier Guilyardi wrote:
> [...]
>>>> +
>>>> +    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.
>> Okay, I've completely changed this. My experience with lock free ringbuffers
>> shows that it's hard to tell what is safe and what is not from a theoretical
>> point of view (and there are many such point of views...).
>>
>> As of JACK 0.116, the jack ringbuffer has been thoroughly unit tested and fixed
>> (see the dozens of posts on LAD, LAU and jack-devel), on single/multi cpu, and
>> especially on weakly memory ordered ones (such as PowerPC SMP).
>>
>> So I found a way to avoid superfluous copy operations, using 2 jack ringbuffers.
>> The idea is simple: audio_read_packet() sends newly allocated empty packets
>> through one ringbuffer. Then process_callback() retrieve one, directly copy and
>> interleave data into it, and send it back through another ringbuffer.
> 
> Iam sure this works but it is certainly not the simplest solution, that is i
> belive my suggestion needs less code also mine needs just one buffer not 2

It depends on what you call simple.

Coding such IPC mechanism using uint8_t and stuff is a subtle thing, with 
potential subtle bugs.

What you call simple IMO is smaller, and potentially faster, code, even if that 
implies much more complexity.

In this very case, I believe your approach is dangerous, because you drastically 
reduce maintainability. Your code may well be bug-free, but even a skilled 
developer might introduce a non-obvious bug, in a later change. Plus, it's hard 
to test because it might behave differently on different cpu architectures.

What is my approach then? I'm using a "brick", the jack ringbuffer, which I know 
to be well tested, just as I would use a semaphore or a mutex, (sort of) blindly 
trusting them. I'm dealing with complexity, but it is well encapsulated.

What is the expected result? Stability now, and because of maintainability: long 
term stability. Which is the top 1 in my priorities.

And I don't think the slight increase in performance (if any) of your solution 
has the same weight as that.

--
   Olivier