Re: [FFmpeg-devel] [PATCH v2] avformat/pcm: decrease delay when reading PCM streams.

[Tomas Härdin]

On 2018-02-15 10:17, Philipp M. Scholl wrote:

On Wed, Feb 14, 2018 at 03:41:18PM +0100, Tomas Härdin wrote:
[..snip..]

FFMAX(codec->sample_rate/25, 1) would be nicer


agree.


+size = FFMIN(size, RAW_SAMPLES * codec->block_align);
+size = 1 << ff_log2(size);
-size= RAW_SAMPLES*s->streams[0]->codecpar->block_align;
   if (size <= 0)
   return AVERROR(EINVAL);

This will never be true since ff_log() always returns >= 0 which makes size

= 1. I suggest putting it before the 1 << ff_log2.

  This can only happen when block_align == 0. So, a check à la

   if (codec->block_align <= 0)
 return AVERROR(EINVAL);

  before calculating the size, should suffice?


That should catch all of them yes. I wish we could prove things like 
this in C like in Ada/SPARK. Alas..


/Tomas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2] avformat/pcm: decrease delay when reading PCM streams.

[Philipp M. Scholl]

On Thu, Feb 15, 2018 at 01:54:17AM +0100, Michael Niedermayer wrote:
[..snip..]
> breaks fate
>
 I was unsure about the ramifications of a changed blocksize. Maybe you can
comment on that?

> 
> [...]
> > --- a/tests/ref/seek/lavf-alaw
> > +++ b/tests/ref/seek/lavf-alaw
> > @@ -1,53 +1,53 @@
> > -ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size: 
> >  1024
> > +ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size: 
> >   128
> >  ret: 0 st:-1 flags:0  ts:-1.00
> > -ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size: 
> >  1024
> > +ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size: 
> >   128
> >  ret: 0 st:-1 flags:1  ts: 1.894167
> > -ret: 0 st: 0 flags:1 dts: 1.894150 pts: 1.894150 pos:  41766 size: 
> >  1024
> > +ret: 0 st: 0 flags:1 dts: 1.894150 pts: 1.894150 pos:  41766 size: 
> >   128
> >  ret: 0 st: 0 flags:0  ts: 0.788345
> > -ret: 0 st: 0 flags:1 dts: 0.788345 pts: 0.788345 pos:  17383 size: 
> >  1024
> > +ret: 0 st: 0 flags:1 dts: 0.788345 pts: 0.788345 pos:  17383 size: 
> >   128
> >  ret: 0 st: 0 flags:1  ts:-0.317506
> > -ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size: 
> >  1024
> > +ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size: 
> >   128
> >  ret: 0 st:-1 flags:0  ts: 2.576668
> >  ret:-EOF
> 
> --- ./tests/ref/seek/lavf-alaw  2018-02-15 01:43:47.387759133 +0100
> +++ tests/data/fate/seek-lavf-alaw  2018-02-15 01:49:09.979762980 +0100
> @@ -1,53 +1,53 @@
> -ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
> 128
> +ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
> 512
>  ret: 0 st:-1 flags:0  ts:-1.00
> -ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
> 128
> +ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
> 512
>  ret: 0 st:-1 flags:1  ts: 1.894167
> -ret: 0 st: 0 flags:1 dts: 1.894150 pts: 1.894150 pos:  41766 size:   
> 128
> +ret: 0 st: 0 flags:1 dts: 1.894150 pts: 1.894150 pos:  41766 size:   
> 512
>  ret: 0 st: 0 flags:0  ts: 0.788345
> -ret: 0 st: 0 flags:1 dts: 0.788345 pts: 0.788345 pos:  17383 size:   
> 128
> +ret: 0 st: 0 flags:1 dts: 0.788345 pts: 0.788345 pos:  17383 size:   
> 512
>  ret: 0 st: 0 flags:1  ts:-0.317506
> -ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
> 128
> +ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
> 512
>  ret: 0 st:-1 flags:0  ts: 2.576668
>  ret:-EOF
> 
> [...]
> -- 
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Opposition brings concord. Out of discord comes the fairest harmony.
> -- Heraclitus



> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2] avformat/pcm: decrease delay when reading PCM streams.

[Philipp M. Scholl]

On Wed, Feb 14, 2018 at 03:41:18PM +0100, Tomas Härdin wrote:
[..snip..]
> FFMAX(codec->sample_rate/25, 1) would be nicer
>
agree.

> > +size = FFMIN(size, RAW_SAMPLES * codec->block_align);
> > +size = 1 << ff_log2(size);
> > -size= RAW_SAMPLES*s->streams[0]->codecpar->block_align;
> >   if (size <= 0)
> >   return AVERROR(EINVAL);
> 
> This will never be true since ff_log() always returns >= 0 which makes size
> >= 1. I suggest putting it before the 1 << ff_log2.
> 
 This can only happen when block_align == 0. So, a check à la

  if (codec->block_align <= 0)
return AVERROR(EINVAL);

 before calculating the size, should suffice?

> > -ret= av_get_packet(s->pb, pkt, size);
> > +ret = av_get_packet(s->pb, pkt, size);
> 
> Doesn't looks like it belongs, but it makes all the assignments align so
> whatever
> 
> /Tomas
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2] avformat/pcm: decrease delay when reading PCM streams.

[Michael Niedermayer]

On Wed, Feb 14, 2018 at 03:11:54PM +0100, Philipp M. Scholl wrote:
> Hi there,
> 
>  thanks for the feedback. I made the following changes to the patch:
> 
>  - hard-code to 40ms delay. Wasn't really sure what a good value is. But given
>that the is mainly for encoding low-rate sensor data in audio streams,
>together with a video recording, 40ms should be fine in most cases.
> 
>  - fix issue when sample rate is smaller (<100, now 25).
> 
>  - use ff_log2 for calculating a power of two read size. A power of two to 
> make
>efficient use of I/O buffers.
> 
>  - remove the superfluous check for multiple input streams. PCM decoders are
>always single-streams, right?!
> 
>  - remove cosmetic changes, and remove readsize hunk :)
> 
>  any comments?
> 
> cheers,
>  -phil
> 
>  The blocksize of the PCM decoder is hard-coded. This creates
> unnecessary delay when reading low-rate (<100Hz) streams. This creates
> issues when multiplexing multiple streams, since other inputs are only
> opened/read after a low-rate input block was completely read.
> 
>  This patch decreases the blocksize for low-rate inputs, so
> approximately a block is read every 40ms. This decreases the startup
> delay when multiplexing inputs with different rates.
> 
> Signed-off-by: Philipp M. Scholl 
> ---
>  libavformat/pcm.c   | 14 ---
>  tests/ref/seek/acodec-pcm-f32be | 54 
> -
>  tests/ref/seek/acodec-pcm-f64be | 54 
> -
>  tests/ref/seek/lavf-alaw| 44 -
>  tests/ref/seek/lavf-au  | 30 +++
>  tests/ref/seek/lavf-mov | 44 -
>  tests/ref/seek/lavf-mulaw   | 44 -
>  tests/ref/seek/lavf-voc | 22 -
>  8 files changed, 157 insertions(+), 149 deletions(-)

breaks fate


[...]
> --- a/tests/ref/seek/lavf-alaw
> +++ b/tests/ref/seek/lavf-alaw
> @@ -1,53 +1,53 @@
> -ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:  
> 1024
> +ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
> 128
>  ret: 0 st:-1 flags:0  ts:-1.00
> -ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:  
> 1024
> +ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
> 128
>  ret: 0 st:-1 flags:1  ts: 1.894167
> -ret: 0 st: 0 flags:1 dts: 1.894150 pts: 1.894150 pos:  41766 size:  
> 1024
> +ret: 0 st: 0 flags:1 dts: 1.894150 pts: 1.894150 pos:  41766 size:   
> 128
>  ret: 0 st: 0 flags:0  ts: 0.788345
> -ret: 0 st: 0 flags:1 dts: 0.788345 pts: 0.788345 pos:  17383 size:  
> 1024
> +ret: 0 st: 0 flags:1 dts: 0.788345 pts: 0.788345 pos:  17383 size:   
> 128
>  ret: 0 st: 0 flags:1  ts:-0.317506
> -ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:  
> 1024
> +ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
> 128
>  ret: 0 st:-1 flags:0  ts: 2.576668
>  ret:-EOF

--- ./tests/ref/seek/lavf-alaw  2018-02-15 01:43:47.387759133 +0100
+++ tests/data/fate/seek-lavf-alaw  2018-02-15 01:49:09.979762980 +0100
@@ -1,53 +1,53 @@
-ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
128
+ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
512
 ret: 0 st:-1 flags:0  ts:-1.00
-ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
128
+ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
512
 ret: 0 st:-1 flags:1  ts: 1.894167
-ret: 0 st: 0 flags:1 dts: 1.894150 pts: 1.894150 pos:  41766 size:   
128
+ret: 0 st: 0 flags:1 dts: 1.894150 pts: 1.894150 pos:  41766 size:   
512
 ret: 0 st: 0 flags:0  ts: 0.788345
-ret: 0 st: 0 flags:1 dts: 0.788345 pts: 0.788345 pos:  17383 size:   
128
+ret: 0 st: 0 flags:1 dts: 0.788345 pts: 0.788345 pos:  17383 size:   
512
 ret: 0 st: 0 flags:1  ts:-0.317506
-ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
128
+ret: 0 st: 0 flags:1 dts: 0.00 pts: 0.00 pos:  0 size:   
512
 ret: 0 st:-1 flags:0  ts: 2.576668
 ret:-EOF

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

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2] avformat/pcm: decrease delay when reading PCM streams.

[Tomas Härdin]

On 2018-02-14 15:11, Philipp M. Scholl wrote:

Hi there,

  thanks for the feedback. I made the following changes to the patch:

  - hard-code to 40ms delay. Wasn't really sure what a good value is. But given
that the is mainly for encoding low-rate sensor data in audio streams,
together with a video recording, 40ms should be fine in most cases.

  - fix issue when sample rate is smaller (<100, now 25).

  - use ff_log2 for calculating a power of two read size. A power of two to make
efficient use of I/O buffers.

  - remove the superfluous check for multiple input streams. PCM decoders are
always single-streams, right?!


There are certainly formats which support multiple audio streams. MOV 
and MXF comes to mind. Anywhere you need multiple languages.. But I see 
the patch doesn't change anything having to do with stream count anyway



diff --git a/libavformat/pcm.c b/libavformat/pcm.c
index 806f91b6b1..48c3c09bb4 100644
--- a/libavformat/pcm.c
+++ b/libavformat/pcm.c
@@ -28,13 +28,21 @@
  
  int ff_pcm_read_packet(AVFormatContext *s, AVPacket *pkt)

  {
-int ret, size;
+int ret, size = INT_MAX;
+AVCodecParameters *codec = s->streams[0]->codecpar;
+
+/*
+ * Compute read size to complete a read every 40ms.  Clamp to RAW_SAMPLES 
if
+ * larger. Use power of two as readsize for I/O efficiency.
+ */
+size = (codec->sample_rate/25 | 1) * codec->block_align;


FFMAX(codec->sample_rate/25, 1) would be nicer


+size = FFMIN(size, RAW_SAMPLES * codec->block_align);
+size = 1 << ff_log2(size);
  
-size= RAW_SAMPLES*s->streams[0]->codecpar->block_align;

  if (size <= 0)
  return AVERROR(EINVAL);


This will never be true since ff_log() always returns >= 0 which makes 
size >= 1. I suggest putting it before the 1 << ff_log2.


  
-ret= av_get_packet(s->pb, pkt, size);

+ret = av_get_packet(s->pb, pkt, size);


Doesn't looks like it belongs, but it makes all the assignments align so 
whatever


/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel