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.
> 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.
> 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?
Thank you, Carl Eugen
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
