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

[Michael Niedermayer]

On Sat, Mar 07, 2009 at 02:10:40PM +0100, Olivier Guilyardi wrote:
> 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.

What you didnt consider though is that we will need a similar buffer for OSS
and possibly others, the jack ringbuffer cant be used for these.

because we cant use jacks ringbuffer we need our own no matter what amount of
extra bugfixing that involves, using jacks ringbuffer in one piece of code 
and our own in another is no gain it just means more code& more bugs.

and after rethinking the code, iam pretty sure any such buffer will need
memory barriers on some cpus

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

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- 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/20090307/5732acbf/attachment.pgp>