[FFmpeg-devel] [PATCH] libavdevice: JACK demuxer
Michael Niedermayer
michaelni
Fri Feb 27 23:35:08 CET 2009
On Fri, Feb 27, 2009 at 09:42:44PM +0100, Olivier Guilyardi wrote:
> M?ns Rullg?rd wrote:
> > Olivier Guilyardi <list at samalyse.com> writes:
> >
> >> Index: configure
> >> ===================================================================
> >> --- configure (revision 17646)
> >> +++ configure (working copy)
> >> @@ -1095,6 +1095,8 @@
> >> bktr_demuxer_deps_any="dev_bktr_ioctl_bt848_h machine_ioctl_bt848_h
> >> dev_video_bktr_ioctl_bt848_h dev_ic_bt8xx_h"
> >> dirac_demuxer_deps="dirac_parser"
> >> dv1394_demuxer_deps="dv1394 dv_demuxer"
> >> +jack_demuxer_deps="jack_jack_h"
> >> +jack_demuxer_extralibs="-ljack -lpthread -lrt"
> >> libdc1394_demuxer_deps="libdc1394"
> >> libnut_demuxer_deps="libnut"
> >> libnut_muxer_deps="libnut"
> >> @@ -2111,6 +2113,8 @@
> >> check_header alsa/asoundlib.h &&
> >> check_lib2 alsa/asoundlib.h snd_pcm_htimestamp -lasound
> >>
> >> +check_header jack/jack.h
> >> +
> >> # deal with the X11 frame grabber
> >> enabled x11grab &&
> >> check_header X11/Xlib.h &&
> >
> > Why don't you check_libs for -ljack?
>
> Because I'm not used to hand crafted configure scripts ;) And I usually let
> pkg-config do the dirty job.
>
> Attached is the version 0.5 of the patch, which fixes this, and a couple of
> other issues: the client member of PrivateData is now marked volatile, and when
> polling times out a bogus timeout value was printed.
first,review by patcheck:
trailing whitespace
ffmpeg-r17647-jack-0.5.patch:18:+check_header jack/jack.h &&
x==0 / x!=0 can be simplified to !x / x
ffmpeg-r17647-jack-0.5.patch:168:+ if (jack_activate(self->client) != 0) {
ffmpeg-r17647-jack-0.5.patch:203:+ if ((test = start_jack(context, params)) != 0)
static prototype, maybe you should reorder your functions
ffmpeg-r17647-jack-0.5.patch:106:+static void create_ringbuffers(PrivateData *self);
ffmpeg-r17647-jack-0.5.patch:107:+static int start_jack(AVFormatContext *context, AVFormatParameters *params);
ffmpeg-r17647-jack-0.5.patch:108:+static void stop_jack(PrivateData *self);
ffmpeg-r17647-jack-0.5.patch:110:+static int audio_read_header(AVFormatContext *context, AVFormatParameters *params);
ffmpeg-r17647-jack-0.5.patch:111:+static int audio_read_packet(AVFormatContext *context, AVPacket *pkt);
ffmpeg-r17647-jack-0.5.patch:112:+static int audio_read_close(AVFormatContext *context);
ffmpeg-r17647-jack-0.5.patch:114:+static int process_callback(jack_nframes_t nframes, void *arg);
ffmpeg-r17647-jack-0.5.patch:115:+static void shutdown_callback(void *arg);
ffmpeg-r17647-jack-0.5.patch:116:+static int xrun_callback(void *arg);
Non static with no ff_/av_ prefix
ffmpeg-r17647-jack-0.5.patch:343:+AVInputFormat jack_demuxer = {
missing } prior to else
ffmpeg-r17647-jack-0.5.patch:243:+ else if (info.size < nframes)
Non doxy comments
ffmpeg-r17647-jack-0.5.patch:63:+#define _BSD_SOURCE 1 // for usleep() from unistd.h
--
ffmpeg-r17647-jack-0.5.patch-77-+
ffmpeg-r17647-jack-0.5.patch-78-+// Size of the internal ringbuffer as a number of jack buffers
ffmpeg-r17647-jack-0.5.patch:79:+#define RING_NBUFFERS 4
ffmpeg-r17647-jack-0.5.patch-80-+
Missing changelog entry (ignore if minor change)
now mine:
[...]
> Index: libavdevice/jack_audio.c
> ===================================================================
> --- libavdevice/jack_audio.c (revision 0)
> +++ libavdevice/jack_audio.c (revision 0)
[...]
> +#define _BSD_SOURCE 1 // for usleep() from unistd.h
> +#include "config.h"
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <jack/jack.h>
> +#include <jack/ringbuffer.h>
that are many headers, are all needed?
[...]
> +typedef struct {
> + jack_client_t * volatile client;
> + jack_nframes_t sample_rate;
> + double frame_ms;
time related things should be calculated exactly, doubles are not,
besides they result in differences between platforms
[...]
> + int rb_size;
> +
> + rb_size = RING_NBUFFERS * sizeof(PacketControlInfo);
declaration & init can be merged
[...]
> + self->nports = params->channels;
> + self->ports = av_malloc(self->nports * sizeof(*self->ports));
> + self->buffer_size = jack_get_buffer_size(self->client);
> + self->period = self->buffer_size * self->frame_ms;
these assignments can be vertically aligned prettier
[...]
> +static void stop_jack(PrivateData *self)
> +{
> + if (self->client) {
> + jack_deactivate(self->client);
> + jack_client_close(self->client);
> + }
> + jack_ringbuffer_free(self->ctl_rb);
> + jack_ringbuffer_free(self->data_rb);
> + av_free(self->ports);
i generally prefer av_freep() as its safer against mistakes
[...]
> +static int process_callback(jack_nframes_t nframes, void *arg)
> +{
> + int i;
> + PrivateData *self = arg;
> + void * buffer;
> + PacketControlInfo info;
> + double latency, cycle_delay;
> +
> + if (!self->client)
> + return 0;
> +
> + if (jack_ringbuffer_write_space(self->ctl_rb) >= sizeof(info)) {
> + info.size = jack_ringbuffer_write_space(self->data_rb) / sizeof(float) / self->nports;
> +
> + if (info.size > nframes)
> + info.size = nframes;
> + else if (info.size < nframes)
> + self->overrun |= DATA_OVERRUN;
> +
> + if (info.size > 0) {
> + latency = 0;
> + for (i = 0; i < self->nports; i++) {
> + latency += jack_port_get_total_latency(self->client, self->ports[i]);
> + buffer = jack_port_get_buffer(self->ports[i], info.size);
> + jack_ringbuffer_write(self->data_rb, (char *) buffer, info.size * sizeof(float));
> + }
> +
> + latency = latency / self->nports;
> + cycle_delay = jack_frames_since_cycle_start(self->client);
> + info.pts = av_gettime() - (latency + cycle_delay) * self->frame_ms;
does jack not provide a timestamp for the returned audio?
[...]
> + while ((timeout > 0) && self->client
> + && (jack_ringbuffer_read_space(self->ctl_rb) < sizeof(info))) {
> + usleep(self->period);
> + timeout -= self->period;
> + }
as has been said the use of usleep() is not pretty
also no blocking should happen when AVFMT_FLAG_NONBLOCK is set
[...]
> + for (i = 0; i < self->nports; i++) {
> + jack_ringbuffer_read(self->data_rb, (char *) self->channel_buffer,
> + info.size * sizeof(float));
> + for (j = 0; j < info.size; j++) {
> + output_data[j * self->nports + i] = self->channel_buffer[j];
> + }
> + }
useless copy?
why not allocate a packet (or several) and read directly into them?
[..]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- 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/20090227/fa6fa782/attachment.pgp>
More information about the ffmpeg-devel
mailing list