On Mon, Jul 24, 2017 at 02:54:01AM +0200, Carl Eugen Hoyos wrote:
> 2017-07-24 2:46 GMT+02:00 Tyler Jones <tdjones...@gmail.com>:
> > On Mon, Jul 24, 2017 at 01:52:20AM +0200, Carl Eugen Hoyos wrote:
> >> 2017-07-24 0:33 GMT+02:00 Tyler Jones <tdjones...@gmail.com>:
> >> > Vorbis I specification requires that the version number as well as the
> >> > window and transform types in the setup header be equal to 0.
> >> >
> >> > Signed-off-by: Tyler Jones <tdjones...@gmail.com>
> >> > ---
> >> > libavcodec/vorbisdec.c | 18 +++++++++++++++---
> >> > 1 file changed, 15 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> >> > index 2a4f482031..f9c3848c4e 100644
> >> > --- a/libavcodec/vorbisdec.c
> >> > +++ b/libavcodec/vorbisdec.c
> >> > @@ -898,8 +898,16 @@ static int
> >> > vorbis_parse_setup_hdr_modes(vorbis_context *vc)
> >> > vorbis_mode *mode_setup = &vc->modes[i];
> >> >
> >> > mode_setup->blockflag = get_bits1(gb);
> >> > - mode_setup->windowtype = get_bits(gb, 16); //FIXME check
> >> > - mode_setup->transformtype = get_bits(gb, 16); //FIXME check
> >> > + mode_setup->windowtype = get_bits(gb, 16);
> >> > + if (mode_setup->windowtype) {
> >> > + av_log(vc->avctx, AV_LOG_ERROR, "Invalid window type,
> >> > must equal 0.\n");
> >> > + return AVERROR_INVALIDDATA;
> >>
> >> Does this fix anything?
> >>
> >> By default, FFmpeg decoders should not (and, more so, should not
> >> suddenly start to) reject files that can be decoded without any
> >> effort.
> >> Or are such files already unplayable, the error message was
> >> just missing?
> >>
> >> You can reject such files for strict conformance mode.
> >>
> >> Carl Eugen
> >
> > I'll defer to your judgement, but this is how the specifications defines it:
> >
> > (4.2.4 -- Modes)
> > verify ranges; zero is the only legal value in Vorbis I for
> > [vorbis_mode_windowtype]
> > and [vorbis_mode_transformtype]. [vorbis_mode_mapping] must not be
> > greater than the
> > highest number mapping in use. Any illegal values render the stream
> > undecodable.
>
> My mail was not meant to imply that the values you reject
> are valid.My point was that the spec declares the stream undecodable, not to prove that they are invalid. I communicated that poorly. > > These values are unused in the decoder and otherwise ignored in the > > specification. > > I may misunderstand this but an unused or ignored value > should never be a reason to reject an input stream by > default. > > > What is even the value of storing these values beyond a temporary value? > > > I believed that it is important to check these values so that an encoder > > cannot produce > > a stream that comes out of sync and end up failing a later test anyways. > > I don't understand how this argument is related to a decoder > patch. > > In any case: Please add a check for > "avctx->strict_std_compliance >= FF_COMPLIANCE_STRICT" > to make it possible to reject such "invalid" files without breaking > playback of such files for unexpecting users. I'll do that instead. > > In the interest of consistency, there are several identical cases where > > values > > have the potential to make the stream 'undecodable' even if they have no > > impact > > on the behavior of the decoder. In all of these other cases, the decoding > > quits > > immediately. Should these be reverted to only log an error message and not > > return error values? > > From a quick look at git log, these checks were not introduced lately or > am I wrong? You are correct, these cases have behaved the way they do for years. It was a genuine question however, would it be prefered to log these similar errors and quit decoding only when concerned about strict compliance? If so, I'll do that instead. I was just following the convention in the file and mistakenly believed it followed best practices, but I should've first checked against other decoders since it has been left alone for so long. Thanks, Tyler Jones
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
