[01/10] alsa_stream: port changes made on xawtv3

Message ID	1315322996-10576-1-git-send-email-mchehab@redhat.com (mailing list archive)
State	New, archived
Headers	show Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter2.kernel.org (8.14.4/8.14.4) with ESMTP id p86FUC3B006040 for <patchwork-media@patchwork.kernel.org>; Tue, 6 Sep 2011 15:30:12 GMT From: Mauro Carvalho Chehab <mchehab@redhat.com> To: Devin Heitmueller <dheitmueller@kernellabs.com> Cc: linux-media@vger.kernel.org, Mauro Carvalho Chehab <mchehab@redhat.com> Subject: [PATCH 01/10] alsa_stream: port changes made on xawtv3 Date: Tue, 6 Sep 2011 12:29:47 -0300 Message-Id: <1315322996-10576-1-git-send-email-mchehab@redhat.com> Sender: linux-media-owner@vger.kernel.org Precedence: bulk

Mauro Carvalho Chehab Sept. 6, 2011, 3:29 p.m. UTC

There are several issues with the original alsa_stream code that got
fixed on xawtv3, made by me and by Hans de Goede. Basically, the
code were re-written, in order to follow the alsa best practises.

Backport the changes from xawtv, in order to make it to work on a
wider range of V4L and sound adapters.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 src/alsa_stream.c |  629 ++++++++++++++++++++++++++++++-----------------------
 src/alsa_stream.h |    6 +-
 src/tvtime.c      |    6 +-
 3 files changed, 363 insertions(+), 278 deletions(-)

Mauro Carvalho Chehab Sept. 6, 2011, 3:40 p.m. UTC | #1

Hi Devin,

Em 06-09-2011 12:29, Mauro Carvalho Chehab escreveu:
> There are several issues with the original alsa_stream code that got
> fixed on xawtv3, made by me and by Hans de Goede. Basically, the
> code were re-written, in order to follow the alsa best practises.
> 
> Backport the changes from xawtv, in order to make it to work on a
> wider range of V4L and sound adapters.

FYI, just flooded your mailbox with 10 patches for tvtime. ;)

I'm wanting to test some things with tvtime on one of my testboxes, but some
of my cards weren't working with the alsa streaming, due to a few bugs that
were solved on xawtv fork.

So, I decided to backport it to tvtime and recompile the Fedora package for it.
That's where the other 9 patches come ;)

Basically, after applying this series of 10 patches, we can just remove all
patches from Fedora, making life easier for distro maintainers (as the same
thing is probably true on other distros - at least one of the Fedora patches
came from Debian, from the fedora git logs).

One important thing for distros is to have a tarball with the latest version
hosted on a public site, so I've increased the version to 1.0.3 and I'm
thinking on storing a copy of it at linuxtv, just like we do with xawtv3.

If you prefer, all patches are also on my tvtime git tree, at:
	http://git.linuxtv.org/mchehab/tvtime.git

Thanks,
Mauro

> 
> Signed-off-by: Mauro Carvalho Chehab<mchehab@redhat.com>
> ---
>   src/alsa_stream.c |  629 ++++++++++++++++++++++++++++++-----------------------
>   src/alsa_stream.h |    6 +-
>   src/tvtime.c      |    6 +-
>   3 files changed, 363 insertions(+), 278 deletions(-)
> 
> diff --git a/src/alsa_stream.c b/src/alsa_stream.c
> index 2243b02..b6a41a5 100644
> --- a/src/alsa_stream.c
> +++ b/src/alsa_stream.c
> @@ -6,6 +6,9 @@
>    *  Derived from the alsa-driver test tool latency.c:
>    *    Copyright (c) by Jaroslav Kysela<perex@perex.cz>
>    *
> + *  Copyright (c) 2011 - Mauro Carvalho Chehab<mchehab@redhat.com>
> + *	Ported to xawtv, with bug fixes and improvements
> + *
>    *  This program is free software; you can redistribute it and/or modify
>    *  it under the terms of the GNU General Public License as published by
>    *  the Free Software Foundation; either version 2 of the License, or
> @@ -32,8 +35,16 @@
>   #include<alsa/asoundlib.h>
>   #include<sys/time.h>
>   #include<math.h>
> +#include "alsa_stream.h"
> +
> +/* Private vars to control alsa thread status */
> +static int alsa_is_running = 0;
> +static int stop_alsa = 0;
> 
> +/* Error handlers */
>   snd_output_t *output = NULL;
> +FILE *error_fp;
> +int verbose = 0;
> 
>   struct final_params {
>       int bufsize;
> @@ -42,403 +53,435 @@ struct final_params {
>       int channels;
>   };
> 
> -int setparams_stream(snd_pcm_t *handle,
> -		     snd_pcm_hw_params_t *params,
> -		     snd_pcm_format_t format,
> -		     int channels,
> -		     int rate,
> -		     const char *id)
> +static int setparams_stream(snd_pcm_t *handle,
> +			    snd_pcm_hw_params_t *params,
> +			    snd_pcm_format_t format,
> +			    int channels,
> +			    const char *id)
>   {
>       int err;
> -    unsigned int rrate;
> 
>       err = snd_pcm_hw_params_any(handle, params);
>       if (err<  0) {
> -	printf("Broken configuration for %s PCM: no configurations available: %s\n", snd_strerror(err), id);
> -	return err;
> -    }
> -    err = snd_pcm_hw_params_set_rate_resample(handle, params, 1);
> -    if (err<  0) {
> -	printf("Resample setup failed for %s: %s\n", id, snd_strerror(err));
> +	fprintf(error_fp,
> +		"alsa: Broken configuration for %s PCM: no configurations available: %s\n",
> +		snd_strerror(err), id);
>   	return err;
>       }
> +
>       err = snd_pcm_hw_params_set_access(handle, params,
>   				       SND_PCM_ACCESS_RW_INTERLEAVED);
>       if (err<  0) {
> -	printf("Access type not available for %s: %s\n", id,
> -	       snd_strerror(err));
> +	fprintf(error_fp, "alsa: Access type not available for %s: %s\n", id,
> +		snd_strerror(err));
>   	return err;
>       }
> 
>       err = snd_pcm_hw_params_set_format(handle, params, format);
>       if (err<  0) {
> -	printf("Sample format not available for %s: %s\n", id,
> +	fprintf(error_fp, "alsa: Sample format not available for %s: %s\n", id,
>   	       snd_strerror(err));
>   	return err;
>       }
>       err = snd_pcm_hw_params_set_channels(handle, params, channels);
>       if (err<  0) {
> -	printf("Channels count (%i) not available for %s: %s\n", channels, id,
> -	       snd_strerror(err));
> -	return err;
> -    }
> -    rrate = rate;
> -    err = snd_pcm_hw_params_set_rate_near(handle, params,&rrate, 0);
> -    if (err<  0) {
> -	printf("Rate %iHz not available for %s: %s\n", rate, id,
> -	       snd_strerror(err));
> +	fprintf(error_fp, "alsa: Channels count (%i) not available for %s: %s\n",
> +		channels, id, snd_strerror(err));
>   	return err;
>       }
> -    if ((int)rrate != rate) {
> -	printf("Rate doesn't match (requested %iHz, get %iHz)\n", rate, err);
> -	return -EINVAL;
> -    }
> +
>       return 0;
>   }
> 
> -int setparams_bufsize(snd_pcm_t *handle,
> +static void getparams_periods(snd_pcm_t *handle,
>   		      snd_pcm_hw_params_t *params,
> -		      snd_pcm_hw_params_t *tparams,
> -		      snd_pcm_uframes_t bufsize,
> -		      int period_size,
> +		      unsigned int *usecs,
> +		      unsigned int *count,
> +		      const char *id)
> +{
> +    unsigned min = 0, max = 0;
> +
> +    snd_pcm_hw_params_get_periods_min(params,&min, 0);
> +    snd_pcm_hw_params_get_periods_max(params,&max, 0);
> +    if (min&&  max) {
> +	if (verbose)
> +	    fprintf(error_fp, "alsa: %s periods range between %u and %u. Want: %u\n",
> +		    id, min, max, *count);
> +	if (*count<  min)
> +	    *count = min;
> +	if (*count>  max)
> +	    *count = max;
> +    }
> +
> +    min = max = 0;
> +    snd_pcm_hw_params_get_period_time_min(params,&min, 0);
> +    snd_pcm_hw_params_get_period_time_max(params,&max, 0);
> +    if (min&&  max) {
> +	if (verbose)
> +	    fprintf(error_fp, "alsa: %s period time range between %u and %u. Want: %u\n",
> +		    id, min, max, *usecs);
> +	if (*usecs<  min)
> +	    *usecs = min;
> +	if (*usecs>  max)
> +	    *usecs = max;
> +    }
> +}
> +
> +static int setparams_periods(snd_pcm_t *handle,
> +		      snd_pcm_hw_params_t *params,
> +		      unsigned int *usecs,
> +		      unsigned int *count,
>   		      const char *id)
>   {
>       int err;
> -    snd_pcm_uframes_t periodsize;
> 
> -    snd_pcm_hw_params_copy(params, tparams);
> -    periodsize = bufsize * 2;
> -    err = snd_pcm_hw_params_set_buffer_size_near(handle, params,&periodsize);
> +    err = snd_pcm_hw_params_set_period_time_near(handle, params, usecs, 0);
>       if (err<  0) {
> -	printf("Unable to set buffer size %li for %s: %s\n",
> -	       bufsize * 2, id, snd_strerror(err));
> -	return err;
> +	    fprintf(error_fp, "alsa: Unable to set period time %u for %s: %s\n",
> +		    *usecs, id, snd_strerror(err));
> +	    return err;
>       }
> -    if (period_size>  0)
> -	periodsize = period_size;
> -    else
> -	periodsize /= 2;
> -    err = snd_pcm_hw_params_set_period_size_near(handle, params,&periodsize,
> -						 0);
> +
> +    err = snd_pcm_hw_params_set_periods_near(handle, params, count, 0);
>       if (err<  0) {
> -	printf("Unable to set period size %li for %s: %s\n", periodsize, id,
> -	       snd_strerror(err));
> +	fprintf(error_fp, "alsa: Unable to set %u periods for %s: %s\n",
> +		*count, id, snd_strerror(err));
>   	return err;
>       }
> +
> +    if (verbose)
> +	fprintf(error_fp, "alsa: %s period set to %u periods of %u time\n",
> +		id, *count, *usecs);
> +
>       return 0;
>   }
> 
> -int setparams_set(snd_pcm_t *handle,
> -		  snd_pcm_hw_params_t *params,
> -		  snd_pcm_sw_params_t *swparams,
> -		  const char *id)
> +static int setparams_set(snd_pcm_t *handle,
> +			 snd_pcm_hw_params_t *params,
> +			 snd_pcm_sw_params_t *swparams,
> +			 snd_pcm_uframes_t start_treshold,
> +			 const char *id)
>   {
>       int err;
> 
>       err = snd_pcm_hw_params(handle, params);
>       if (err<  0) {
> -	printf("Unable to set hw params for %s: %s\n", id, snd_strerror(err));
> +	fprintf(error_fp, "alsa: Unable to set hw params for %s: %s\n",
> +		id, snd_strerror(err));
>   	return err;
>       }
>       err = snd_pcm_sw_params_current(handle, swparams);
>       if (err<  0) {
> -	printf("Unable to determine current swparams for %s: %s\n", id,
> -	       snd_strerror(err));
> +	fprintf(error_fp, "alsa: Unable to determine current swparams for %s: %s\n",
> +		id, snd_strerror(err));
>   	return err;
>       }
> -    err = snd_pcm_sw_params_set_start_threshold(handle, swparams, 0x7fffffff);
> +    err = snd_pcm_sw_params_set_start_threshold(handle, swparams,
> +						start_treshold);
>       if (err<  0) {
> -	printf("Unable to set start threshold mode for %s: %s\n", id,
> -	       snd_strerror(err));
> +	fprintf(error_fp, "alsa: Unable to set start threshold mode for %s: %s\n",
> +		id, snd_strerror(err));
>   	return err;
>       }
> 
>       err = snd_pcm_sw_params_set_avail_min(handle, swparams, 4);
>       if (err<  0) {
> -	printf("Unable to set avail min for %s: %s\n", id, snd_strerror(err));
> +	fprintf(error_fp, "alsa: Unable to set avail min for %s: %s\n",
> +		id, snd_strerror(err));
>   	return err;
>       }
>       err = snd_pcm_sw_params(handle, swparams);
>       if (err<  0) {
> -	printf("Unable to set sw params for %s: %s\n", id, snd_strerror(err));
> +	fprintf(error_fp, "alsa: Unable to set sw params for %s: %s\n",
> +		id, snd_strerror(err));
>   	return err;
>       }
>       return 0;
>   }
> 
> -int setparams(snd_pcm_t *phandle, snd_pcm_t *chandle, snd_pcm_format_t format,
> -	      struct final_params *negotiated)
> +static int setparams(snd_pcm_t *phandle, snd_pcm_t *chandle,
> +		     snd_pcm_format_t format,
> +		     int latency, int allow_resample,
> +		     struct final_params *negotiated)
>   {
> -    int rate = 48000;
> -    int latency_min = 600;		/* in frames / 2 */
> -    int channels = 2;
> -    int latency = latency_min - 4;
> -    int bufsize = latency;
> -    int err, last_bufsize = bufsize;
> -    snd_pcm_hw_params_t *pt_params, *ct_params;
> -    snd_pcm_hw_params_t *p_params, *c_params;
> +    int i;
> +    unsigned ratep, ratec = 0;
> +    unsigned ratemin = 32000, ratemax = 96000, val;
> +    int err, channels = 2;
> +    snd_pcm_hw_params_t *p_hwparams, *c_hwparams;
>       snd_pcm_sw_params_t *p_swparams, *c_swparams;
> -    snd_pcm_uframes_t p_size, c_size, p_psize, c_psize;
> -    unsigned int p_time, c_time;
> -
> -    snd_pcm_hw_params_alloca(&p_params);
> -    snd_pcm_hw_params_alloca(&c_params);
> -    snd_pcm_hw_params_alloca(&pt_params);
> -    snd_pcm_hw_params_alloca(&ct_params);
> +    snd_pcm_uframes_t c_size, p_psize, c_psize;
> +    /* Our latency is 2 periods (in usecs) */
> +    unsigned int c_periods = 2, p_periods;
> +    unsigned int c_periodtime, p_periodtime;
> +
> +    snd_pcm_hw_params_alloca(&p_hwparams);
> +    snd_pcm_hw_params_alloca(&c_hwparams);
>       snd_pcm_sw_params_alloca(&p_swparams);
>       snd_pcm_sw_params_alloca(&c_swparams);
> -    if ((err = setparams_stream(phandle, pt_params, format, channels, rate,
> -				"playback"))<  0) {
> -	printf("Unable to set parameters for playback stream: %s\n", snd_strerror(err));
> -	return 1;
> -    }
> -    if ((err = setparams_stream(chandle, ct_params, format, channels, rate,
> -				"capture"))<  0) {
> -	printf("Unable to set parameters for playback stream: %s\n", snd_strerror(err));
> +
> +    if (setparams_stream(phandle, p_hwparams, format, channels, "playback"))
>   	return 1;
> -    }
> 
> -  __again:
> -    if (last_bufsize == bufsize)
> -	bufsize += 4;
> -    last_bufsize = bufsize;
> +    if (setparams_stream(chandle, c_hwparams, format, channels, "capture"))
> +	return 1;
> 
> -    if ((err = setparams_bufsize(phandle, p_params, pt_params, bufsize, 0,
> -				 "playback"))<  0) {
> -	printf("Unable to set sw parameters for playback stream: %s\n",
> -	       snd_strerror(err));
> -	return -1;
> -    }
> -    if ((err = setparams_bufsize(chandle, c_params, ct_params, bufsize, 0,
> -				 "capture"))<  0) {
> -	printf("Unable to set sw parameters for playback stream: %s\n",
> -	       snd_strerror(err));
> -	return -1;
> +    if (allow_resample) {
> +	err = snd_pcm_hw_params_set_rate_resample(chandle, c_hwparams, 1);
> +	if (err<  0) {
> +	    fprintf(error_fp, "alsa: Resample setup failed: %s\n", snd_strerror(err));
> +	    return 1;
> +	} else if (verbose)
> +	   fprintf(error_fp, "alsa: Resample enabled.\n");
> +    }
> +
> +    err = snd_pcm_hw_params_get_rate_min(c_hwparams,&ratemin, 0);
> +    if (err>= 0&&  verbose)
> +	fprintf(error_fp, "alsa: Capture min rate is %d\n", ratemin);
> +    err = snd_pcm_hw_params_get_rate_max(c_hwparams,&ratemax, 0);
> +    if (err>= 0&&  verbose)
> +	fprintf(error_fp, "alsa: Capture max rate is %u\n", ratemax);
> +
> +    err = snd_pcm_hw_params_get_rate_min(p_hwparams,&val, 0);
> +    if (err>= 0) {
> +	if (verbose)
> +	    fprintf(error_fp, "alsa: Playback min rate is %u\n", val);
> +	if (val>  ratemin)
> +		ratemin = val;
> +    }
> +    err = snd_pcm_hw_params_get_rate_max(p_hwparams,&val, 0);
> +    if (err>= 0) {
> +	if (verbose)
> +	    fprintf(error_fp, "alsa: Playback max rate is %u\n", val);
> +	if (val<  ratemax)
> +		ratemax = val;
> +    }
> +
> +    if (verbose)
> +	fprintf(error_fp, "alsa: Will search a common rate between %u and %u\n",
> +		ratemin, ratemax);
> +
> +    for (i = ratemin; i<= ratemax; i+= 100) {
> +	err = snd_pcm_hw_params_set_rate_near(chandle, c_hwparams,&i, 0);
> +	if (err)
> +	    continue;
> +	ratec = i;
> +	ratep = i;
> +	err = snd_pcm_hw_params_set_rate_near(phandle, p_hwparams,&ratep, 0);
> +	if (err)
> +	    continue;
> +	if (ratep == ratec)
> +	    break;
> +	if (verbose)
> +	    fprintf(error_fp,
> +		    "alsa: Failed to set to %u: capture wanted %u, playback wanted %u%s\n",
> +		    i, ratec, ratep,
> +		    allow_resample ? " with resample enabled": "");
>       }
> 
> -    snd_pcm_hw_params_get_period_size(p_params,&p_psize, NULL);
> -    if (p_psize>  (unsigned int)bufsize)
> -	bufsize = p_psize;
> -
> -    snd_pcm_hw_params_get_period_size(c_params,&c_psize, NULL);
> -    if (c_psize>  (unsigned int)bufsize)
> -	bufsize = c_psize;
> +    if (err<  0) {
> +	fprintf(error_fp, "alsa: Failed to set a supported rate: %s\n",
> +		snd_strerror(err));
> +	return 1;
> +    }
> +    if (ratep != ratec) {
> +	if (verbose || allow_resample)
> +	    fprintf(error_fp,
> +		    "alsa: Couldn't find a rate that it is supported by both playback and capture\n");
> +	return 2;
> +    }
> +    if (verbose)
> +	fprintf(error_fp, "alsa: Using Rate %d\n", ratec);
> +
> +    /* Negociate period parameters */
> +
> +    c_periodtime = latency * 1000 / c_periods;
> +    getparams_periods(chandle, c_hwparams,&c_periodtime,&c_periods, "capture");
> +    p_periods = c_periods * 2;
> +    p_periodtime = c_periodtime;
> +    getparams_periods(phandle, p_hwparams,&p_periodtime,&p_periods, "playback");
> +    c_periods = p_periods / 2;
> +
> +    /*
> +     * Some playback devices support a very limited periodtime range. If the user needs to
> +     * use a higher latency to avoid overrun/underrun, use an alternate algorithm of incresing
> +     * the number of periods, to archive the needed latency
> +     */
> +    if (p_periodtime<  c_periodtime) {
> +	c_periodtime = p_periodtime;
> +	c_periods = round (latency * 1000.0 / c_periodtime + 0.5);
> +	getparams_periods(chandle, c_hwparams,&c_periodtime,&c_periods, "capture");
> +	p_periods = c_periods * 2;
> +	p_periodtime = c_periodtime;
> +	getparams_periods(phandle, p_hwparams,&p_periodtime,&p_periods, "playback");
> +	c_periods = p_periods / 2;
> +    }
> +
> +    if (setparams_periods(chandle, c_hwparams,&c_periodtime,&c_periods, "capture"))
> +	return 1;
> 
> -    snd_pcm_hw_params_get_period_time(p_params,&p_time, NULL);
> -    snd_pcm_hw_params_get_period_time(c_params,&c_time, NULL);
> +    /* Note we use twice as much periods for the playback buffer, since we
> +       will get a period size near the requested time and we don't want it to
> +       end up smaller then the capture buffer as then we could end up blocking
> +       on writing to it. Note we will configure the playback dev to start
> +       playing as soon as it has 2 capture periods worth of data, so this
> +       won't influence latency */
> +    if (setparams_periods(phandle, p_hwparams,&p_periodtime,&p_periods, "playback"))
> +	return 1;
> 
> -    if (p_time != c_time)
> -	goto __again;
> +    snd_pcm_hw_params_get_period_size(p_hwparams,&p_psize, NULL);
> +    snd_pcm_hw_params_get_period_size(c_hwparams,&c_psize, NULL);
> +    snd_pcm_hw_params_get_buffer_size(c_hwparams,&c_size);
> 
> -    snd_pcm_hw_params_get_buffer_size(p_params,&p_size);
> -    if (p_psize * 2<  p_size)
> -	goto __again;
> -    snd_pcm_hw_params_get_buffer_size(c_params,&c_size);
> -    if (c_psize * 2<  c_size)
> -	goto __again;
> +    latency = c_periods * c_psize;
> +    if (setparams_set(phandle, p_hwparams, p_swparams, latency, "playback"))
> +	return 1;
> 
> -    if ((err = setparams_set(phandle, p_params, p_swparams, "playback"))<  0) {
> -	printf("Unable to set sw parameters for playback stream: %s\n",
> -	       snd_strerror(err));
> -	return -1;
> -    }
> -    if ((err = setparams_set(chandle, c_params, c_swparams, "capture"))<  0) {
> -	printf("Unable to set sw parameters for playback stream: %s\n",
> -	       snd_strerror(err));
> -	return -1;
> -    }
> +    if (setparams_set(chandle, c_hwparams, c_swparams, c_psize, "capture"))
> +	return 1;
> 
>       if ((err = snd_pcm_prepare(phandle))<  0) {
> -	printf("Prepare error: %s\n", snd_strerror(err));
> -	return -1;
> +	fprintf(error_fp, "alsa: Prepare error: %s\n", snd_strerror(err));
> +	return 1;
>       }
> 
> -#ifdef SHOW_ALSA_DEBUG
> -    printf("final config\n");
> -    snd_pcm_dump_setup(phandle, output);
> -    snd_pcm_dump_setup(chandle, output);
> -    printf("Parameters are %iHz, %s, %i channels\n", rate,
> -	   snd_pcm_format_name(format), channels);
> -    fflush(stdout);
> -#endif
> +    if (verbose) {
> +	fprintf(error_fp, "alsa: Negociated configuration:\n");
> +	snd_pcm_dump_setup(phandle, output);
> +	snd_pcm_dump_setup(chandle, output);
> +	fprintf(error_fp, "alsa: Parameters are %iHz, %s, %i channels\n",
> +		ratep, snd_pcm_format_name(format), channels);
> +	fprintf(error_fp, "alsa: Set bitrate to %u%s, buffer size is %u\n", ratec,
> +		allow_resample ? " with resample enabled at playback": "",
> +		(unsigned int)c_size);
> +    }
> 
> -    negotiated->bufsize = bufsize;
> -    negotiated->rate = rate;
> +    negotiated->bufsize = c_size;
> +    negotiated->rate = ratep;
>       negotiated->channels = channels;
> -    negotiated->latency = bufsize;
> +    negotiated->latency = latency;
>       return 0;
>   }
> 
> -void setscheduler(void)
> -{
> -    struct sched_param sched_param;
> -
> -    if (sched_getparam(0,&sched_param)<  0) {
> -	printf("Scheduler getparam failed...\n");
> -	return;
> -    }
> -    sched_param.sched_priority = sched_get_priority_max(SCHED_RR);
> -    if (!sched_setscheduler(0, SCHED_RR,&sched_param)) {
> -	printf("Scheduler set to Round Robin with priority %i...\n", sched_param.sched_priority);
> -	fflush(stdout);
> -	return;
> -    }
> -    printf("!!!Scheduler set to Round Robin with priority %i FAILED!!!\n", sched_param.sched_priority);
> -}
> -
> -snd_pcm_sframes_t readbuf(snd_pcm_t *handle, char *buf, long len,
> -			  size_t *frames, size_t *max)
> +/* Read up to len frames */
> +static snd_pcm_sframes_t readbuf(snd_pcm_t *handle, char *buf, long len)
>   {
>       snd_pcm_sframes_t r;
> 
>       r = snd_pcm_readi(handle, buf, len);
> -    if (r<  0) {
> -	return r;
> +    if (r<  0&&  r != -EAGAIN) {
> +	r = snd_pcm_recover(handle, r, 0);
> +	if (r<  0)
> +	    fprintf(error_fp, "alsa: overrun recover error: %s\n", snd_strerror(r));
>       }
> -
> -    if (r>  0) {
> -	*frames += r;
> -	if ((long)*max<  r)
> -	    *max = r;
> -    }
> -
>       return r;
>   }
> 
> -snd_pcm_sframes_t writebuf(snd_pcm_t *handle, char *buf, long len,
> -			   size_t *frames)
> +/* Write len frames (note not up to len, but all of len!) */
> +static snd_pcm_sframes_t writebuf(snd_pcm_t *handle, char *buf, long len)
>   {
>       snd_pcm_sframes_t r;
> 
> -    while (len>  0) {
> +    while (1) {
>   	r = snd_pcm_writei(handle, buf, len);
> +	if (r == len)
> +	    return 0;
>   	if (r<  0) {
> -	    return r;
> +	    r = snd_pcm_recover(handle, r, 0);
> +	    if (r<  0) {
> +		fprintf(error_fp, "alsa: underrun recover error: %s\n",
> +			snd_strerror(r));
> +		return r;
> +	    }
>   	}
> -
>   	buf += r * 4;
>   	len -= r;
> -	*frames += r;
> -    }
> -    return 0;
> -}
> -
> -int startup_capture(snd_pcm_t *phandle, snd_pcm_t *chandle,
> -		    snd_pcm_format_t format, char *buffer, int latency,
> -		    int channels)
> -{
> -    size_t frames_out;
> -    int err;
> -
> -    frames_out = 0;
> -    if (snd_pcm_format_set_silence(format, buffer, latency*channels)<  0) {
> -	fprintf(stderr, "silence error\n");
> -	return 1;
> -    }
> -    if (writebuf(phandle, buffer, latency,&frames_out)<  0) {
> -	fprintf(stderr, "write error\n");
> -	return 1;
> +	snd_pcm_wait(handle, 100);
>       }
> -    if (writebuf(phandle, buffer, latency,&frames_out)<  0) {
> -	fprintf(stderr, "write error\n");
> -	return 1;
> -    }
> -
> -    if ((err = snd_pcm_start(chandle))<  0) {
> -	printf("Go error: %s\n", snd_strerror(err));
> -	return 1;
> -    }
> -    return 0;
>   }
> 
> -int tvtime_alsa_stream(const char *pdevice, const char *cdevice)
> +static int alsa_stream(const char *pdevice, const char *cdevice, int latency)
>   {
>       snd_pcm_t *phandle, *chandle;
>       char *buffer;
>       int err;
>       ssize_t r;
> -    size_t frames_in, frames_out, in_max;
>       struct final_params negotiated;
> -    int ret = 0;
>       snd_pcm_format_t format = SND_PCM_FORMAT_S16_LE;
> +    char pdevice_new[32];
> 
> -    err = snd_output_stdio_attach(&output, stdout, 0);
> +    err = snd_output_stdio_attach(&output, error_fp, 0);
>       if (err<  0) {
> -	printf("Output failed: %s\n", snd_strerror(err));
> +	fprintf(error_fp, "alsa: Output failed: %s\n", snd_strerror(err));
>   	return 0;
>       }
> 
> -//    setscheduler();
> -
> -    printf("Playback device is %s\n", pdevice);
> -    printf("Capture device is %s\n", cdevice);
> -
>       /* Open the devices */
>       if ((err = snd_pcm_open(&phandle, pdevice, SND_PCM_STREAM_PLAYBACK,
> -			    SND_PCM_NONBLOCK))<  0) {
> -	printf("Cannot open ALSA Playback device %s: %s\n", pdevice,
> -	       snd_strerror(err));
> +			    0))<  0) {
> +	fprintf(error_fp, "alsa: Cannot open playback device %s: %s\n",
> +		pdevice, snd_strerror(err));
>   	return 0;
>       }
>       if ((err = snd_pcm_open(&chandle, cdevice, SND_PCM_STREAM_CAPTURE,
>   			    SND_PCM_NONBLOCK))<  0) {
> -	printf("Cannot open ALSA Capture device %s: %s\n",
> -	       cdevice, snd_strerror(err));
> +	fprintf(error_fp, "alsa: Cannot open capture device %s: %s\n",
> +		cdevice, snd_strerror(err));
>   	return 0;
>       }
> 
> -    frames_in = frames_out = 0;
> -    if (setparams(phandle, chandle, format,&negotiated)<  0) {
> -	printf("setparams failed\n");
> +    err = setparams(phandle, chandle, format, latency, 0,&negotiated);
> +
> +    /* Try to use plughw instead, as it allows emulating speed */
> +    if (err == 2&&  strncmp(pdevice, "hw", 2) == 0) {
> +
> +	snd_pcm_close(phandle);
> +
> +	sprintf(pdevice_new, "plug%s", pdevice);
> +	pdevice = pdevice_new;
> +	if (verbose)
> +	    fprintf(error_fp, "alsa: Trying %s for playback\n", pdevice);
> +	if ((err = snd_pcm_open(&phandle, pdevice, SND_PCM_STREAM_PLAYBACK,
> +				0))<  0) {
> +	    fprintf(error_fp, "alsa: Cannot open playback device %s: %s\n",
> +		    pdevice, snd_strerror(err));
> +	}
> +
> +	err = setparams(phandle, chandle, format, latency, 1,&negotiated);
> +    }
> +
> +    if (err != 0) {
> +	fprintf(error_fp, "alsa: setparams failed\n");
>   	return 1;
>       }
> 
>       buffer = malloc((negotiated.bufsize * snd_pcm_format_width(format) / 8)
>   		    * negotiated.channels);
>       if (buffer == NULL) {
> -	printf("Failed allocating buffer for audio\n");
> +	fprintf(error_fp, "alsa: Failed allocating buffer for audio\n");
>   	return 0;
> -
> -    }
> -    if ((err = snd_pcm_link(chandle, phandle))<  0) {
> -	printf("Streams link error: %d %s\n", err, snd_strerror(err));
> -	return 1;
>       }
> 
> -    startup_capture(phandle, chandle, format, buffer, negotiated.latency,
> -		    negotiated.channels);
> -
> -    while (1) { 			
> -	in_max = 0;
> -
> +    /*
> +     * Buffering delay is due for capture and for playback, so we
> +     * need to multiply it by two.
> +     */
> +    fprintf(error_fp,
> +	    "alsa: stream started from %s to %s (%i Hz, buffer delay = %.2f ms)\n",
> +	    cdevice, pdevice, negotiated.rate,
> +	    negotiated.latency * 1000.0 / negotiated.rate);
> +
> +    alsa_is_running = 1;
> +
> +    while (!stop_alsa) {
> +	/* We start with a read and not a wait to auto(re)start the capture */
> +	r = readbuf(chandle, buffer, negotiated.bufsize);
> +	if (r == 0)   /* Succesfully recovered from an overrun? */
> +	    continue; /* Force restart of capture stream */
> +	if (r>  0)
> +	    writebuf(phandle, buffer, r);
>   	/* use poll to wait for next event */
> -	ret = snd_pcm_wait(chandle, 1000);
> -	if (ret<  0) {
> -	    if ((err = snd_pcm_recover(chandle, ret, 0))<  0) {
> -		fprintf(stderr, "xrun: recover error: %s",
> -			snd_strerror(err));
> -		break;
> -	    }
> -
> -	    /* Restart capture */
> -	    startup_capture(phandle, chandle, format, buffer,
> -			    negotiated.latency, negotiated.channels);
> -	    continue;
> -	} else if (ret == 0) {
> -	    /* Timed out */
> -	    continue;
> -	}
> -
> -	if ((r = readbuf(chandle, buffer, negotiated.latency,&frames_in,
> -			&in_max))>  0) {
> -	    if (writebuf(phandle, buffer, r,&frames_out)<  0) {
> -		startup_capture(phandle, chandle, format, buffer,
> -				negotiated.latency, negotiated.channels);
> -	    }
> -	} else if (r<  0) {
> -	    startup_capture(phandle, chandle, format, buffer,
> -			    negotiated.latency, negotiated.channels);
> -	}
> +	snd_pcm_wait(chandle, 1000);
>       }
> 
>       snd_pcm_drop(chandle);
> @@ -451,28 +494,55 @@ int tvtime_alsa_stream(const char *pdevice, const char *cdevice)
> 
>       snd_pcm_close(phandle);
>       snd_pcm_close(chandle);
> +
> +    alsa_is_running = 0;
>       return 0;
>   }
> 
>   struct input_params {
> -    const char *pdevice;
> -    const char *cdevice;
> +    char *pdevice;
> +    char *cdevice;
> +    int latency;
>   };
> 
> -void *tvtime_alsa_thread_entry(void *whatever)
> +static void *alsa_thread_entry(void *whatever)
>   {
>       struct input_params *inputs = (struct input_params *) whatever;
> -    tvtime_alsa_stream(inputs->pdevice, inputs->cdevice);
> +
> +    if (verbose)
> +	fprintf(error_fp, "alsa: starting copying alsa stream from %s to %s\n",
> +		inputs->cdevice, inputs->pdevice);
> +    alsa_stream(inputs->pdevice, inputs->cdevice, inputs->latency);
> +    fprintf(error_fp, "alsa: stream stopped\n");
> +
> +    free(inputs->pdevice);
> +    free(inputs->cdevice);
> +    free(inputs);
> +
> +    return NULL;
>   }
> 
> -int tvtime_alsa_thread_startup(const char *pdevice, const char *cdevice)
> +/*************************************************************************
> + Public functions
> + *************************************************************************/
> +
> +int alsa_thread_startup(const char *pdevice, const char *cdevice, int latency,
> +			FILE *__error_fp, int __verbose)
>   {
>       int ret;
>       pthread_t thread;
>       struct input_params *inputs = malloc(sizeof(struct input_params));
> 
> +    if (__error_fp)
> +	error_fp = __error_fp;
> +    else
> +	error_fp = stderr;
> +
> +    verbose = __verbose;
> +
> +
>       if (inputs == NULL) {
> -	printf("failed allocating memory for ALSA inputs\n");
> +	fprintf(error_fp, "alsa: failed allocating memory for inputs\n");
>   	return 0;
>       }
> 
> @@ -484,18 +554,27 @@ int tvtime_alsa_thread_startup(const char *pdevice, const char *cdevice)
> 
>       inputs->pdevice = strdup(pdevice);
>       inputs->cdevice = strdup(cdevice);
> +    inputs->latency = latency;
> +
> +    if (alsa_is_running) {
> +       stop_alsa = 1;
> +       while ((volatile int)alsa_is_running)
> +	       usleep(10);
> +    }
> +
> +    stop_alsa = 0;
> 
>       ret = pthread_create(&thread, NULL,
> -			&tvtime_alsa_thread_entry, (void *) inputs);
> +			&alsa_thread_entry, (void *) inputs);
>       return ret;
>   }
> 
> -#ifdef TVTIME_ALSA_DEBUGGING
> -/* This allows the alsa_stream.c to be a standalone binary for debugging */
> - int main(int argc, char *argv[])
> +void alsa_thread_stop(void)
> +{
> +	stop_alsa = 1;
> +}
> +
> +int alsa_thread_is_running(void)
>   {
> -    char *pdevice = "hw:0,0";
> -    char *cdevice = "hw:1,0";
> -    tvtime_alsa_stream(pdevice, cdevice);
> +	return alsa_is_running;
>   }
> -#endif
> diff --git a/src/alsa_stream.h b/src/alsa_stream.h
> index 8572c8b..c68fd6d 100644
> --- a/src/alsa_stream.h
> +++ b/src/alsa_stream.h
> @@ -1 +1,5 @@
> -int tvtime_alsa_thread_startup(char *pdevice, char *cdevice);
> +int alsa_thread_startup(const char *pdevice, const char *cdevice, int latency,
> +			FILE *__error_fp,
> +			int __verbose);
> +void alsa_thread_stop(void);
> +int alsa_thread_is_running(void);
> diff --git a/src/tvtime.c b/src/tvtime.c
> index 75257d1..d9066cc 100644
> --- a/src/tvtime.c
> +++ b/src/tvtime.c
> @@ -1256,8 +1256,10 @@ int tvtime_main( rtctimer_t *rtctimer, int read_stdin, int realtime,
>       }
> 
>       /* Setup the ALSA streaming device */
> -    tvtime_alsa_thread_startup(config_get_alsa_outputdev( ct ),
> -			       config_get_alsa_inputdev( ct ) );
> +    alsa_thread_startup(config_get_alsa_outputdev( ct ),
> +			config_get_alsa_inputdev( ct ),
> +			30,		/* FIXME: Add a var to adjust latency */
> +			stderr, verbose );
> 
>       /* Setup the speedy calls. */
>       setup_speedy_calls( mm_accel(), verbose );

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Devin Heitmueller Sept. 6, 2011, 4:24 p.m. UTC | #2

On Tue, Sep 6, 2011 at 11:40 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Hi Devin,
>
> Em 06-09-2011 12:29, Mauro Carvalho Chehab escreveu:
>> There are several issues with the original alsa_stream code that got
>> fixed on xawtv3, made by me and by Hans de Goede. Basically, the
>> code were re-written, in order to follow the alsa best practises.
>>
>> Backport the changes from xawtv, in order to make it to work on a
>> wider range of V4L and sound adapters.
>
> FYI, just flooded your mailbox with 10 patches for tvtime. ;)
>
> I'm wanting to test some things with tvtime on one of my testboxes, but some
> of my cards weren't working with the alsa streaming, due to a few bugs that
> were solved on xawtv fork.
>
> So, I decided to backport it to tvtime and recompile the Fedora package for it.
> That's where the other 9 patches come ;)
>
> Basically, after applying this series of 10 patches, we can just remove all
> patches from Fedora, making life easier for distro maintainers (as the same
> thing is probably true on other distros - at least one of the Fedora patches
> came from Debian, from the fedora git logs).
>
> One important thing for distros is to have a tarball with the latest version
> hosted on a public site, so I've increased the version to 1.0.3 and I'm
> thinking on storing a copy of it at linuxtv, just like we do with xawtv3.
>
> If you prefer, all patches are also on my tvtime git tree, at:
>        http://git.linuxtv.org/mchehab/tvtime.git
>
> Thanks,
> Mauro

Hi Mauro,

Funny you should send these along today.  Last Friday I was actually
poking around at the Fedora tvtime repo because I was curious how they
had dealt with the V4L1 support issue (whether they were using my
patch removing v4l1 or some variant).

I've actually pulled in Fedora patches in the past (as you can see
from the hg repo), and it has always been my intention to do it for
the other distros as well (e.g. debian/Ubuntu).  So I appreciate your
having sent these along.

I'll pull these in this week, do some testing to make sure nothing
serious got broken, and work to spin a 1.0.3 toward the end of the
week.  Given the number of features/changes, and how long it's been
since the last formal release, I was considering calling it 1.1.0
instead though.

I've been thinking for a while that perhaps the project should be
renamed (or I considered prepending "kl" onto the front resulting in
it being called "kl-tvtime").  This isn't out of vanity but rather my
concern that the fork will get confused with the original project (for
example, I believe Ubuntu actually already calls their modified tree
tvtime 1.0.3).  I'm open to suggestions in this regards.

Devin

Hans de Goede Sept. 6, 2011, 6:19 p.m. UTC | #3

Hi,

On 09/06/2011 06:24 PM, Devin Heitmueller wrote:

<snip>

> I've been thinking for a while that perhaps the project should be
> renamed (or I considered prepending "kl" onto the front resulting in
> it being called "kl-tvtime").  This isn't out of vanity but rather my
> concern that the fork will get confused with the original project (for
> example, I believe Ubuntu actually already calls their modified tree
> tvtime 1.0.3).  I'm open to suggestions in this regards.

I think that what should be done is contact the debian / ubuntu maintainers,
get any interesting fixes they have which the kl version misses merged,
and then just declare the kl version as being the new official upstream
(with the blessing of the debian / ubuntu guys, and if possible also
with the blessing of the original authors).

This would require kl git to be open to others for pushing, or we
could move the tree to git.linuxtv.org (which I assume may be
easier then for you to make the necessary changes to give
others push rights on kl.org).

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michael Krufky Sept. 6, 2011, 6:35 p.m. UTC | #4

On Tue, Sep 6, 2011 at 2:19 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 09/06/2011 06:24 PM, Devin Heitmueller wrote:
>
> <snip>
>
>> I've been thinking for a while that perhaps the project should be
>> renamed (or I considered prepending "kl" onto the front resulting in
>> it being called "kl-tvtime").  This isn't out of vanity but rather my
>> concern that the fork will get confused with the original project (for
>> example, I believe Ubuntu actually already calls their modified tree
>> tvtime 1.0.3).  I'm open to suggestions in this regards.
>
> I think that what should be done is contact the debian / ubuntu maintainers,
> get any interesting fixes they have which the kl version misses merged,
> and then just declare the kl version as being the new official upstream
> (with the blessing of the debian / ubuntu guys, and if possible also
> with the blessing of the original authors).
>
> This would require kl git to be open to others for pushing, or we
> could move the tree to git.linuxtv.org (which I assume may be
> easier then for you to make the necessary changes to give
> others push rights on kl.org).

Hans,

Everybody is welcome to contribute to open source projects, but global
contribution doesn't mean that a given server be opened up to commits
by the general public.  You should feel free to push to your own git
tree hosted on linuxtv.org (or any public git server, for that matter)
and send pull requests to Devin Heitmueller, who is currently
maintaining the kernellabs version of tvtime.

Regards,

Michael Krufky
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab Sept. 6, 2011, 6:40 p.m. UTC | #5

Em 06-09-2011 13:24, Devin Heitmueller escreveu:
> On Tue, Sep 6, 2011 at 11:40 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com>  wrote:
>> Hi Devin,
>>
>> Em 06-09-2011 12:29, Mauro Carvalho Chehab escreveu:
>>> There are several issues with the original alsa_stream code that got
>>> fixed on xawtv3, made by me and by Hans de Goede. Basically, the
>>> code were re-written, in order to follow the alsa best practises.
>>>
>>> Backport the changes from xawtv, in order to make it to work on a
>>> wider range of V4L and sound adapters.
>>
>> FYI, just flooded your mailbox with 10 patches for tvtime. ;)
>>
>> I'm wanting to test some things with tvtime on one of my testboxes, but some
>> of my cards weren't working with the alsa streaming, due to a few bugs that
>> were solved on xawtv fork.
>>
>> So, I decided to backport it to tvtime and recompile the Fedora package for it.
>> That's where the other 9 patches come ;)
>>
>> Basically, after applying this series of 10 patches, we can just remove all
>> patches from Fedora, making life easier for distro maintainers (as the same
>> thing is probably true on other distros - at least one of the Fedora patches
>> came from Debian, from the fedora git logs).
>>
>> One important thing for distros is to have a tarball with the latest version
>> hosted on a public site, so I've increased the version to 1.0.3 and I'm
>> thinking on storing a copy of it at linuxtv, just like we do with xawtv3.
>>
>> If you prefer, all patches are also on my tvtime git tree, at:
>>         http://git.linuxtv.org/mchehab/tvtime.git
>>
>> Thanks,
>> Mauro
> 
> Hi Mauro,
> 
> Funny you should send these along today.  Last Friday I was actually
> poking around at the Fedora tvtime repo because I was curious how they
> had dealt with the V4L1 support issue (whether they were using my
> patch removing v4l1 or some variant).

Well, right time then ;)
> 
> I've actually pulled in Fedora patches in the past (as you can see
> from the hg repo),

Yes, I saw it. Nice work!

> and it has always been my intention to do it for
> the other distros as well (e.g. debian/Ubuntu).  So I appreciate your
> having sent these along.

It is a good idea to take a look at them. I looked into their repositories
for the xawtv patches and I found some good stuff there.

> I'll pull these in this week, do some testing to make sure nothing
> serious got broken, and work to spin a 1.0.3 toward the end of the
> week.

Great!
> Given the number of features/changes, and how long it's been
> since the last formal release, I was considering calling it 1.1.0
> instead though.

Seems fine for me.

> I've been thinking for a while that perhaps the project should be
> renamed (or I considered prepending "kl" onto the front resulting in
> it being called "kl-tvtime").  This isn't out of vanity but rather my
> concern that the fork will get confused with the original project (for
> example, I believe Ubuntu actually already calls their modified tree
> tvtime 1.0.3).  I'm open to suggestions in this regards.

IMO, I won't rename it. It is a well-known tool, and it is not a new
version, but somebody's else took over its maintainership. I think
you should touch the readme files in order to point to kl.com and to
the places where the tree will be stored.

Em 06-09-2011 15:19, Hans de Goede escreveu:
> Hi,
<snip>
> I think that what should be done is contact the debian / ubuntu maintainers,
> get any interesting fixes they have which the kl version misses merged,
> and then just declare the kl version as being the new official upstream
> (with the blessing of the debian / ubuntu guys, and if possible also
> with the blessing of the original authors).

Agree. I think Devin already tried to contact vektor about that.

> This would require kl git to be open to others for pushing, or we
> could move the tree to git.linuxtv.org (which I assume may be
> easier then for you to make the necessary changes to give
> others push rights on kl.org).

I like this idea too. From my side, it proved to be very useful to be
able to write on both Fedora and upstream repositories on xawtv3. 

I've made already the Fedora changes for tvtime 1.0.3 (in order to test
it on my test boxes), so being able of adding a new release at both
repos at the same time is a good idea.

Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Devin Heitmueller Sept. 6, 2011, 6:41 p.m. UTC | #6

On Tue, Sep 6, 2011 at 2:19 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> I think that what should be done is contact the debian / ubuntu maintainers,
> get any interesting fixes they have which the kl version misses merged,
> and then just declare the kl version as being the new official upstream
> (with the blessing of the debian / ubuntu guys, and if possible also
> with the blessing of the original authors).

It has always been my intention to get the Debian/Ubuntu patches
merged (as well as other distros).  My thoughts behind renaming were
oriented around the notion that that there are more distros out there
than just Fedora/Ubuntu/Debian, but that may be something that isn't
really a concern.  Also, I had no idea whether the distros would
actually switch over to the Kernel Labs version as the official
upstream source, so providing it under a different name would in
theory allow both packages to be available in parallel.

From a practical standpoint, the Ubuntu folks have the original tvtime
tarball and all their changes in one patch, which is clearly a bunch
of patches that are mashed together probably in their build system.  I
need to reach out to them to find where they have an actual SCM tree
or the individual patches.  They've got a bunch of patches which would
be good to get into a single tree (autobuild fixes, cross-compilation,
locale updates, etc).

> This would require kl git to be open to others for pushing, or we
> could move the tree to git.linuxtv.org (which I assume may be
> easier then for you to make the necessary changes to give
> others push rights on kl.org).

Kernel Labs has never really had any real interest in "owning" tvtime.
 I just setup the hg tree in an effort to get all the distro patches
in one place and have something that builds against current kernels
(and on which I can add improvements/fixes without users having to
deal with patches).  At the time there was also nobody who clearly had
the desire to serve as an official maintainer.

In the long term I have no real issue with the LinuxTV group being the
official maintainer of record.  I've got lots of ideas and things I
would like to do to improve tvtime, but in practice I've done a pretty
crappy job of maintaining the source (merging patches, etc) at this
point.

Devin

Hans de Goede Sept. 6, 2011, 6:43 p.m. UTC | #7

Hi,

On 09/06/2011 08:35 PM, Michael Krufky wrote:
> On Tue, Sep 6, 2011 at 2:19 PM, Hans de Goede<hdegoede@redhat.com>  wrote:
>> Hi,
>>
>> On 09/06/2011 06:24 PM, Devin Heitmueller wrote:
>>
>> <snip>
>>
>>> I've been thinking for a while that perhaps the project should be
>>> renamed (or I considered prepending "kl" onto the front resulting in
>>> it being called "kl-tvtime").  This isn't out of vanity but rather my
>>> concern that the fork will get confused with the original project (for
>>> example, I believe Ubuntu actually already calls their modified tree
>>> tvtime 1.0.3).  I'm open to suggestions in this regards.
>>
>> I think that what should be done is contact the debian / ubuntu maintainers,
>> get any interesting fixes they have which the kl version misses merged,
>> and then just declare the kl version as being the new official upstream
>> (with the blessing of the debian / ubuntu guys, and if possible also
>> with the blessing of the original authors).
>>
>> This would require kl git to be open to others for pushing, or we
>> could move the tree to git.linuxtv.org (which I assume may be
>> easier then for you to make the necessary changes to give
>> others push rights on kl.org).
>
> Hans,
>
> Everybody is welcome to contribute to open source projects, but global
> contribution doesn't mean that a given server be opened up to commits
> by the general public.

I didn't write open to commits by the general public, now did I? I wrote
open to commits by others. For most upstream projects it is quite normal
that several people have push rights to the master tree. This actually
is quite a good idea, as it avoids adding a SPOF into the chain. It
means development can continue if one of the maintainers is on vacation
for a a few weeks, or just having a period in his/her life where he
is too busy to actively contribute to a spare time project.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michael Krufky Sept. 6, 2011, 7:11 p.m. UTC | #8

On Tue, Sep 6, 2011 at 2:43 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 09/06/2011 08:35 PM, Michael Krufky wrote:
>>
>> On Tue, Sep 6, 2011 at 2:19 PM, Hans de Goede<hdegoede@redhat.com>  wrote:
>>>
>>> Hi,
>>>
>>> On 09/06/2011 06:24 PM, Devin Heitmueller wrote:
>>>
>>> <snip>
>>>
>>>> I've been thinking for a while that perhaps the project should be
>>>> renamed (or I considered prepending "kl" onto the front resulting in
>>>> it being called "kl-tvtime").  This isn't out of vanity but rather my
>>>> concern that the fork will get confused with the original project (for
>>>> example, I believe Ubuntu actually already calls their modified tree
>>>> tvtime 1.0.3).  I'm open to suggestions in this regards.
>>>
>>> I think that what should be done is contact the debian / ubuntu
>>> maintainers,
>>> get any interesting fixes they have which the kl version misses merged,
>>> and then just declare the kl version as being the new official upstream
>>> (with the blessing of the debian / ubuntu guys, and if possible also
>>> with the blessing of the original authors).
>>>
>>> This would require kl git to be open to others for pushing, or we
>>> could move the tree to git.linuxtv.org (which I assume may be
>>> easier then for you to make the necessary changes to give
>>> others push rights on kl.org).
>>
>> Hans,
>>
>> Everybody is welcome to contribute to open source projects, but global
>> contribution doesn't mean that a given server be opened up to commits
>> by the general public.
>
> I didn't write open to commits by the general public, now did I? I wrote
> open to commits by others. For most upstream projects it is quite normal
> that several people have push rights to the master tree. This actually
> is quite a good idea, as it avoids adding a SPOF into the chain. It
> means development can continue if one of the maintainers is on vacation
> for a a few weeks, or just having a period in his/her life where he
> is too busy to actively contribute to a spare time project.

Hans,

Now I understand -- that's completely reasonable.  It looks like Devin
is happy having the tree hosted on linuxtv.org anyway, so no worries
:-)  Sorry for the misunderstanding.

Best Regards,

Mike Krufky
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab Sept. 6, 2011, 7:12 p.m. UTC | #9

Em 06-09-2011 15:41, Devin Heitmueller escreveu:
> On Tue, Sep 6, 2011 at 2:19 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> I think that what should be done is contact the debian / ubuntu maintainers,
>> get any interesting fixes they have which the kl version misses merged,
>> and then just declare the kl version as being the new official upstream
>> (with the blessing of the debian / ubuntu guys, and if possible also
>> with the blessing of the original authors).
> 
> It has always been my intention to get the Debian/Ubuntu patches
> merged (as well as other distros).  My thoughts behind renaming were
> oriented around the notion that that there are more distros out there
> than just Fedora/Ubuntu/Debian, but that may be something that isn't
> really a concern.  Also, I had no idea whether the distros would
> actually switch over to the Kernel Labs version as the official
> upstream source, so providing it under a different name would in
> theory allow both packages to be available in parallel.
> 
> From a practical standpoint, the Ubuntu folks have the original tvtime
> tarball and all their changes in one patch, which is clearly a bunch
> of patches that are mashed together probably in their build system. I
> need to reach out to them to find where they have an actual SCM tree
> or the individual patches.  They've got a bunch of patches which would
> be good to get into a single tree (autobuild fixes, cross-compilation,
> locale updates, etc).

Yeah, it seems interesting. Maybe we can get something from this place:
	http://packages.qa.debian.org/t/tvtime.html

The maintainer there seems to be: 
	http://qa.debian.org/developer.php?login=bartm@debian.org

>> This would require kl git to be open to others for pushing, or we
>> could move the tree to git.linuxtv.org (which I assume may be
>> easier then for you to make the necessary changes to give
>> others push rights on kl.org).
> 
> Kernel Labs has never really had any real interest in "owning" tvtime.
>  I just setup the hg tree in an effort to get all the distro patches
> in one place and have something that builds against current kernels
> (and on which I can add improvements/fixes without users having to
> deal with patches).  At the time there was also nobody who clearly had
> the desire to serve as an official maintainer.
> 
> In the long term I have no real issue with the LinuxTV group being the
> official maintainer of record.  I've got lots of ideas and things I
> would like to do to improve tvtime, but in practice I've done a pretty
> crappy job of maintaining the source (merging patches, etc) at this
> point.

Putting it on a common place and giving permissions to a group of people
is interesting, as none of us are focused on userspace, so we all
have a very limited amount of time for dealing with userspace applications.

By giving commit rights to a group of developers, it ends that more 
developers will contribute, speeding up the development. 

That was what happened with v4l-utils and, on a minor scale, with xawtv3.

If you're ok with that, I can set a tvtime git repository at LinuxTV, 
cloning the tree I've created there already (it is a pure conversion
of your tree from mercurial into git, if I remove the patches I've
done so far from your clone), giving you the ownership of the new tree,
and marking it as a shared repository.

I have already all set there to allow shared access to the repository
(in opposite to -hg, git works really cool with shared repositories).

We can later add permissions to the developers interested on helping
the tvtime maintenance that you agree to add.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Devin Heitmueller Sept. 6, 2011, 9:18 p.m. UTC | #10

On Tue, Sep 6, 2011 at 3:12 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
>> From a practical standpoint, the Ubuntu folks have the original tvtime
>> tarball and all their changes in one patch, which is clearly a bunch
>> of patches that are mashed together probably in their build system. I
>> need to reach out to them to find where they have an actual SCM tree
>> or the individual patches.  They've got a bunch of patches which would
>> be good to get into a single tree (autobuild fixes, cross-compilation,
>> locale updates, etc).
>
> Yeah, it seems interesting. Maybe we can get something from this place:
>        http://packages.qa.debian.org/t/tvtime.html
>
> The maintainer there seems to be:
>        http://qa.debian.org/developer.php?login=bartm@debian.org

I reached out to the Ubuntu maintainer; we'll see if he gets back to
me.  From what I can tell it seems like Debian is actually taking the
patches from Ubuntu (yes, I realize this is backwards from their
typical process where Ubuntu bases their stuff on Debian).

>> In the long term I have no real issue with the LinuxTV group being the
>> official maintainer of record.  I've got lots of ideas and things I
>> would like to do to improve tvtime, but in practice I've done a pretty
>> crappy job of maintaining the source (merging patches, etc) at this
>> point.
>
> Putting it on a common place and giving permissions to a group of people
> is interesting, as none of us are focused on userspace, so we all
> have a very limited amount of time for dealing with userspace applications.
>
> By giving commit rights to a group of developers, it ends that more
> developers will contribute, speeding up the development.
>
> That was what happened with v4l-utils and, on a minor scale, with xawtv3.
>
> If you're ok with that, I can set a tvtime git repository at LinuxTV,
> cloning the tree I've created there already (it is a pure conversion
> of your tree from mercurial into git, if I remove the patches I've
> done so far from your clone), giving you the ownership of the new tree,
> and marking it as a shared repository.

I have no problem with this.  Let's set it up.

> I have already all set there to allow shared access to the repository
> (in opposite to -hg, git works really cool with shared repositories).

I actually haven't hosted any git repos on linuxtv.org before.  I'm
assuming my ssh public key got copied over from when I was hosting hg
repos there?

> We can later add permissions to the developers interested on helping
> the tvtime maintenance that you agree to add.

Sounds good.

As said earlier, Kernel Labs never really wanted to be the maintainer
for tvtime - we did it because nobody else wanted to (and vektor never
responded to emails I sent him offering to help).  That said, a
community oriented approach is probably the best for everybody
involved.

I'll probably be looking in the next couple of weeks to write some
fresh content for a tvtime website.  The stuff on
tvtime.sourceforge.net is so dated almost none of it still applies.

Thanks,

Devin

Mauro Carvalho Chehab Sept. 6, 2011, 9:50 p.m. UTC | #11

Em 06-09-2011 18:18, Devin Heitmueller escreveu:
> On Tue, Sep 6, 2011 at 3:12 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>>> From a practical standpoint, the Ubuntu folks have the original tvtime
>>> tarball and all their changes in one patch, which is clearly a bunch
>>> of patches that are mashed together probably in their build system. I
>>> need to reach out to them to find where they have an actual SCM tree
>>> or the individual patches.  They've got a bunch of patches which would
>>> be good to get into a single tree (autobuild fixes, cross-compilation,
>>> locale updates, etc).
>>
>> Yeah, it seems interesting. Maybe we can get something from this place:
>>        http://packages.qa.debian.org/t/tvtime.html
>>
>> The maintainer there seems to be:
>>        http://qa.debian.org/developer.php?login=bartm@debian.org
> 
> I reached out to the Ubuntu maintainer; we'll see if he gets back to
> me.  From what I can tell it seems like Debian is actually taking the
> patches from Ubuntu (yes, I realize this is backwards from their
> typical process where Ubuntu bases their stuff on Debian).

Good!

>>> In the long term I have no real issue with the LinuxTV group being the
>>> official maintainer of record.  I've got lots of ideas and things I
>>> would like to do to improve tvtime, but in practice I've done a pretty
>>> crappy job of maintaining the source (merging patches, etc) at this
>>> point.
>>
>> Putting it on a common place and giving permissions to a group of people
>> is interesting, as none of us are focused on userspace, so we all
>> have a very limited amount of time for dealing with userspace applications.
>>
>> By giving commit rights to a group of developers, it ends that more
>> developers will contribute, speeding up the development.
>>
>> That was what happened with v4l-utils and, on a minor scale, with xawtv3.
>>
>> If you're ok with that, I can set a tvtime git repository at LinuxTV,
>> cloning the tree I've created there already (it is a pure conversion
>> of your tree from mercurial into git, if I remove the patches I've
>> done so far from your clone), giving you the ownership of the new tree,
>> and marking it as a shared repository.
> 
> I have no problem with this.  Let's set it up.

Ok. The repository is here:
	http://git.linuxtv.org/tvtime.git

In thesis, everything is set for group usage. Please let me know if
you experience any troubles with it.

>> I have already all set there to allow shared access to the repository
>> (in opposite to -hg, git works really cool with shared repositories).
> 
> I actually haven't hosted any git repos on linuxtv.org before.  I'm
> assuming my ssh public key got copied over from when I was hosting hg
> repos there?

The same key is used, whatever you're committing to cvs, hg or git. The
maintenance application for git is called git-menu.

>> We can later add permissions to the developers interested on helping
>> the tvtime maintenance that you agree to add.
> 
> Sounds good.

From my side, I'm interested on helping with it.

When I have some time, I'd like to fix a few issues with it. 

For example, there's a local cable operator that broadcasts some channels 
with PAL/M and others with NTSC/M (not a big deal for STBs and TV sets, as
almost all support both standards here).

However, tvtime, needs to be restarted every time it changes from one to 
the other, and it is not possible to set a per-channel standard. To be 
worse, when tvtime is restarted, it doesn't honor the "-d" option, with
means that it will open my laptop's webcam instead of the TV card.
> 
> As said earlier, Kernel Labs never really wanted to be the maintainer
> for tvtime - we did it because nobody else wanted to (and vektor never
> responded to emails I sent him offering to help).  That said, a
> community oriented approach is probably the best for everybody
> involved.
> 
> I'll probably be looking in the next couple of weeks to write some
> fresh content for a tvtime website.  The stuff on
> tvtime.sourceforge.net is so dated almost none of it still applies.

Yeah, that makes sense.
> Thanks,
> 
> Devin
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Devin Heitmueller Sept. 7, 2011, 2:58 a.m. UTC | #12

On Tue, Sep 6, 2011 at 11:29 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> There are several issues with the original alsa_stream code that got
> fixed on xawtv3, made by me and by Hans de Goede. Basically, the
> code were re-written, in order to follow the alsa best practises.
>
> Backport the changes from xawtv, in order to make it to work on a
> wider range of V4L and sound adapters.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

Mauro,

What tuners did you test this patch with?  I went ahead and did a git
pull of your patch series into my local git tree, and now my DVC-90
(an em28xx device) is capturing at 32 KHz instead of 48 (this is one
of the snd-usb-audio based devices, not em28xx-alsa).

Note I tested immediately before pulling your patch series and the
audio capture was working fine.

I think this patch series is going in the right direction in general,
but this patch in particular seems to cause a regression.  Is this
something you want to investigate?  I think we need to hold off on
pulling this series into the new tvtime master until this problem is
resolved.

Devin

Mauro Carvalho Chehab Sept. 7, 2011, 3:23 a.m. UTC | #13

Em 06-09-2011 23:58, Devin Heitmueller escreveu:
> On Tue, Sep 6, 2011 at 11:29 AM, Mauro Carvalho Chehab 
> <mchehab@redhat.com> wrote:
>> There are several issues with the original alsa_stream code that
>> got fixed on xawtv3, made by me and by Hans de Goede. Basically,
>> the code were re-written, in order to follow the alsa best
>> practises.
>> 
>> Backport the changes from xawtv, in order to make it to work on a 
>> wider range of V4L and sound adapters.
>> 
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> Mauro,
> 
> What tuners did you test this patch with?

I tested with some em28xx-based devices, like HVR-950 and WinTV USB2.

> I went ahead and did a git pull of your patch series into my local
> git tree, and now my DVC-90 (an em28xx device) is capturing at 32 KHz
> instead of 48 (this is one of the snd-usb-audio based devices, not
> em28xx-alsa).

The new approach tries to match an speed that it is compatible between
the audio and the video card. The algorithm tries first to not use
software interpolation for audio, as it would reduce the audio quality.

If it can't do it without interpolation, it will enable interpolation
and seek again. By default, pulseaudio does interpolation, if you request
it to use a different resolution.

> Note I tested immediately before pulling your patch series and the 
> audio capture was working fine.



Had you test to disable pulseaudio and see what speeds your boards
accept? If you enable verbose mode, you'll see more details about
the device detection.


For example, this is what I get here with hvr-950, calling "tvtime -v":

alsa: starting copying alsa stream from hw:1,0 to hw:0,0
videoinput: Using video4linux2 driver 'em28xx', card 'Hauppauge WinTV HVR 950' (bus usb-0000:00:1d.7-1).
videoinput: Version is 196608, capabilities 5030051.
alsa: Capture min rate is 48000
alsa: Capture max rate is 48000
alsa: Playback min rate is 44100
alsa: Playback max rate is 192000
alsa: Will search a common rate between 48000 and 48000
alsa: Using Rate 48000
alsa: capture periods range between 2 and 98. Want: 2
alsa: capture period time range between 333 and 65334. Want: 15000
alsa: playback periods range between 2 and 32. Want: 4
alsa: playback period time range between 666 and 10922667. Want: 15000
alsa: capture period set to 2 periods of 15000 time
alsa: playback period set to 4 periods of 15333 time
alsa: Negociated configuration:
  stream       : PLAYBACK
  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 48000
  exact rate   : 48000 (48000/1)
  msbits       : 16
  buffer_size  : 2944
  period_size  : 736
  period_time  : 15333
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 736
  period_event : 0
  start_threshold  : 1440
  stop_threshold   : 2944
  silence_threshold: 0
  silence_size : 0
  boundary     : 1543503872
  stream       : CAPTURE
  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 48000
  exact rate   : 48000 (48000/1)
  msbits       : 16
  buffer_size  : 1440
  period_size  : 720
  period_time  : 15000
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 720
  period_event : 0
  start_threshold  : 720
  stop_threshold   : 1440
  silence_threshold: 0
  silence_size : 0
  boundary     : 1509949440
alsa: Parameters are 48000Hz, S16_LE, 2 channels
alsa: Set bitrate to 48000, buffer size is 1440
alsa: stream started from hw:1,0 to hw:0,0 (48000 Hz, buffer delay = 30,00 ms)

And those are the results with WinTV USB2:

videoinput: Using video4linux2 driver 'em28xx', card 'Hauppauge WinTV USB 2' (bus usb-0000:00:1d.7-1).
videoinput: Version is 196608, capabilities 5030041.
alsa: starting copying alsa stream from hw:1,0 to hw:0,0
alsa: Capture min rate is 32000
alsa: Capture max rate is 32000
alsa: Playback min rate is 44100
alsa: Playback max rate is 192000
alsa: Will search a common rate between 44100 and 32000
alsa: Couldn't find a rate that it is supported by both playback and capture
alsa: Trying plughw:0,0 for playback
alsa: Resample enabled.
alsa: Capture min rate is 32000
alsa: Capture max rate is 32000
alsa: Playback min rate is 4000
alsa: Playback max rate is 4294967295
alsa: Will search a common rate between 32000 and 32000
alsa: Using Rate 32000
alsa: capture periods range between 2 and 1024. Want: 2
alsa: capture period time range between 500 and 4096000. Want: 15000
alsa: playback period time range between 333 and 5461334. Want: 15000
alsa: capture period set to 2 periods of 15000 time
alsa: playback period set to 4 periods of 15000 time
alsa: Negociated configuration:
  stream       : PLAYBACK
  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 32000
  exact rate   : 32000 (32000/1)
  msbits       : 16
  buffer_size  : 1920
  period_size  : 480
  period_time  : 15000
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 480
  period_event : 0
  start_threshold  : 960
  stop_threshold   : 1920
  silence_threshold: 0
  silence_size : 0
  boundary     : 503316480
  stream       : CAPTURE
  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 32000
  exact rate   : 32000 (32000/1)
  msbits       : 16
  buffer_size  : 960
  period_size  : 480
  period_time  : 15000
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 480
  period_event : 0
  start_threshold  : 480
  stop_threshold   : 960
  silence_threshold: 0
  silence_size : 0
  boundary     : 2013265920
alsa: Parameters are 32000Hz, S16_LE, 2 channels
alsa: Set bitrate to 32000 with resample enabled at playback, buffer size is 960
alsa: stream started from hw:1,0 to plughw:0,0 (32000 Hz, buffer delay = 30,00 ms)

You should notice that snd-usb-audio only reports 32 kHz speed for this
device.

> I think this patch series is going in the right direction in
> general, but this patch in particular seems to cause a regression.
> Is this something you want to investigate?  I think we need to hold
> off on pulling this series into the new tvtime master until this
> problem is resolved.
> 
> Devin
> 

/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[01/10] alsa_stream: port changes made on xawtv3

Commit Message

Comments

Patch