diff mbox series

[FFmpeg-devel,4/4] lavf/wvdec: remove artificial restrictions on stream parameter changes

Message ID 20200405203241.13033-4-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,1/4] pthread_frame: make sure ff_thread_release_buffer always cleans the frame
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Anton Khirnov April 5, 2020, 8:32 p.m. UTC
They are not forbidden by the specification.
---
 libavformat/wvdec.c | 18 ------------------
 1 file changed, 18 deletions(-)

Comments

David Bryant April 6, 2020, 4:56 a.m. UTC | #1
On 4/5/20 1:32 PM, Anton Khirnov wrote:
> They are not forbidden by the specification.
> ---
>  libavformat/wvdec.c | 18 ------------------
>  1 file changed, 18 deletions(-)
>
> diff --git a/libavformat/wvdec.c b/libavformat/wvdec.c
> index b9fc6a59f9..4159bf1253 100644
> --- a/libavformat/wvdec.c
> +++ b/libavformat/wvdec.c
> @@ -208,24 +208,6 @@ static int wv_read_block_header(AVFormatContext *ctx, AVIOContext *pb)
>      if (!wc->rate)
>          wc->rate   = rate * rate_x;
>  
> -    if (flags && bpp != wc->bpp) {
> -        av_log(ctx, AV_LOG_ERROR,
> -               "Bits per sample differ, this block: %i, header block: %i\n",
> -               bpp, wc->bpp);
> -        return AVERROR_INVALIDDATA;
> -    }
> -    if (flags && !wc->multichannel && chan != wc->chan) {
> -        av_log(ctx, AV_LOG_ERROR,
> -               "Channels differ, this block: %i, header block: %i\n",
> -               chan, wc->chan);
> -        return AVERROR_INVALIDDATA;
> -    }
> -    if (flags && rate != -1 && !(flags & WV_DSD) && rate * rate_x != wc->rate) {
> -        av_log(ctx, AV_LOG_ERROR,
> -               "Sampling rate differ, this block: %i, header block: %i\n",
> -               rate * rate_x, wc->rate);
> -        return AVERROR_INVALIDDATA;
> -    }
>      return 0;
>  }
>  

The specification does not explicitly mention that the data format, the channel count (and layout), and the sample rate cannot
change inside of a WavPack file, but they can't. The first block of a WavPack file that contains audio determines the format of
the entire file, and the documentation will be updated shortly to reflect that. And obviously Konstantin Shishkov was aware of
this, even referring to the first block containing audio as the "header block".

I have no issue with the decoder handling these changes as it might be receiving a WavPack stream from a container that allows
this, but the native WavPack container doesn't. And I actually don't have any problem with this specific change going in because
it doesn't actually break anything. However, it should not be possible to create a WavPack file with format changes such as these.
Anton Khirnov April 6, 2020, 7:59 a.m. UTC | #2
Quoting David Bryant (2020-04-06 06:56:05)
> On 4/5/20 1:32 PM, Anton Khirnov wrote:
> > They are not forbidden by the specification.
> > ---
> >  libavformat/wvdec.c | 18 ------------------
> >  1 file changed, 18 deletions(-)
> >
> > diff --git a/libavformat/wvdec.c b/libavformat/wvdec.c
> > index b9fc6a59f9..4159bf1253 100644
> > --- a/libavformat/wvdec.c
> > +++ b/libavformat/wvdec.c
> > @@ -208,24 +208,6 @@ static int wv_read_block_header(AVFormatContext *ctx, AVIOContext *pb)
> >      if (!wc->rate)
> >          wc->rate   = rate * rate_x;
> >  
> > -    if (flags && bpp != wc->bpp) {
> > -        av_log(ctx, AV_LOG_ERROR,
> > -               "Bits per sample differ, this block: %i, header block: %i\n",
> > -               bpp, wc->bpp);
> > -        return AVERROR_INVALIDDATA;
> > -    }
> > -    if (flags && !wc->multichannel && chan != wc->chan) {
> > -        av_log(ctx, AV_LOG_ERROR,
> > -               "Channels differ, this block: %i, header block: %i\n",
> > -               chan, wc->chan);
> > -        return AVERROR_INVALIDDATA;
> > -    }
> > -    if (flags && rate != -1 && !(flags & WV_DSD) && rate * rate_x != wc->rate) {
> > -        av_log(ctx, AV_LOG_ERROR,
> > -               "Sampling rate differ, this block: %i, header block: %i\n",
> > -               rate * rate_x, wc->rate);
> > -        return AVERROR_INVALIDDATA;
> > -    }
> >      return 0;
> >  }
> >  
> 
> The specification does not explicitly mention that the data format, the channel count (and layout), and the sample rate cannot
> change inside of a WavPack file, but they can't. The first block of a WavPack file that contains audio determines the format of
> the entire file, and the documentation will be updated shortly to reflect that. And obviously Konstantin Shishkov was aware of
> this, even referring to the first block containing audio as the "header block".
> 
> I have no issue with the decoder handling these changes as it might be receiving a WavPack stream from a container that allows
> this, but the native WavPack container doesn't. And I actually don't have any problem with this specific change going in because
> it doesn't actually break anything. However, it should not be possible to create a WavPack file with format changes such as these.

Okay, then maybe I'll drop this patch. My intent was for it to allow
adding tests for wavpack format changes, but I guess that can also be
done with matroska.
diff mbox series

Patch

diff --git a/libavformat/wvdec.c b/libavformat/wvdec.c
index b9fc6a59f9..4159bf1253 100644
--- a/libavformat/wvdec.c
+++ b/libavformat/wvdec.c
@@ -208,24 +208,6 @@  static int wv_read_block_header(AVFormatContext *ctx, AVIOContext *pb)
     if (!wc->rate)
         wc->rate   = rate * rate_x;
 
-    if (flags && bpp != wc->bpp) {
-        av_log(ctx, AV_LOG_ERROR,
-               "Bits per sample differ, this block: %i, header block: %i\n",
-               bpp, wc->bpp);
-        return AVERROR_INVALIDDATA;
-    }
-    if (flags && !wc->multichannel && chan != wc->chan) {
-        av_log(ctx, AV_LOG_ERROR,
-               "Channels differ, this block: %i, header block: %i\n",
-               chan, wc->chan);
-        return AVERROR_INVALIDDATA;
-    }
-    if (flags && rate != -1 && !(flags & WV_DSD) && rate * rate_x != wc->rate) {
-        av_log(ctx, AV_LOG_ERROR,
-               "Sampling rate differ, this block: %i, header block: %i\n",
-               rate * rate_x, wc->rate);
-        return AVERROR_INVALIDDATA;
-    }
     return 0;
 }