Re: [FFmpeg-devel] [PATCH v2] avformat/pcm: decrease delay when reading PCM streams.
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.
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.
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.
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.
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