[FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: Fix get_value return type
Soft Works
softworkz at hotmail.com
Sat Oct 2 15:25:54 EEST 2021
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Saturday, 2 October 2021 13:46
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: Fix
> get_value return type
>
> On Fri, Oct 01, 2021 at 08:44:04PM +0000, Soft Works wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > > Michael Niedermayer
> > > Sent: Friday, 1 October 2021 19:56
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel at ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec:
> Fix
> > > get_value return type
> > >
> > > On Thu, Sep 30, 2021 at 12:11:13PM +0000, Soft Works wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On
> Behalf Of
> > > > > Michael Niedermayer
> > > > > Sent: Thursday, 30 September 2021 14:04
> > > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > > devel at ffmpeg.org>
> > > > > Subject: Re: [FFmpeg-devel] [PATCH v5 2/7]
> libavformat/asfdec:
> > > Fix
> > > > > get_value return type
> > > > >
> > > > > On Thu, Sep 30, 2021 at 01:45:46PM +0200, Michael Niedermayer
> > > wrote:
> > > > > > On Thu, Sep 30, 2021 at 02:58:30AM +0000, Soft Works wrote:
> > > > > > > get_value had a return type of int, which means that
> reading
> > > > > > > QWORDS (case 4) was broken due to truncation of the
> result
> > > from
> > > > > > > avio_rl64().
> > > > > > >
> > > > > > > Signed-off-by: softworkz <softworkz at hotmail.com>
> > > > > > > ---
> > > > > > > v5: Split into pieces as requested
> > > > > > >
> > > > > > > libavformat/asfdec_f.c | 10 +++++-----
> > > > > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > > > > >
> > > > > > > diff --git a/libavformat/asfdec_f.c
> b/libavformat/asfdec_f.c
> > > > > > > index 72ba8b32a0..076b5ab147 100644
> > > > > > > --- a/libavformat/asfdec_f.c
> > > > > > > +++ b/libavformat/asfdec_f.c
> > > > > > > @@ -202,7 +202,7 @@ static int asf_probe(const
> AVProbeData
> > > *pd)
> > > > > > >
> > > > > > > /* size of type 2 (BOOL) is 32bit for "Extended Content
> > > > > Description Object"
> > > > > > > * but 16 bit for "Metadata Object" and "Metadata
> Library
> > > > > Object" */
> > > > > > > -static int get_value(AVIOContext *pb, int type, int
> > > type2_size)
> > > > > > > +static uint64_t get_value(AVIOContext *pb, int type, int
> > > > > type2_size)
> > > > > > > {
> > > > > > > switch (type) {
> > > > > > > case 2:
> > > > > > > @@ -568,9 +568,9 @@ static int
> > > > > asf_read_ext_content_desc(AVFormatContext *s, int64_t size)
> > > > > > > * ASF stream count starts at 1. I am using 0 to
> the
> > > > > container value
> > > > > > > * since it's unused. */
> > > > > > > if (!strcmp(name, "AspectRatioX"))
> > > > > > > - asf->dar[0].num = get_value(s->pb,
> value_type,
> > > 32);
> > > > > > > + asf->dar[0].num = (int)get_value(s->pb,
> > > value_type,
> > > > > 32);
> > > > > > > else if (!strcmp(name, "AspectRatioY"))
> > > > > > > - asf->dar[0].den = get_value(s->pb,
> value_type,
> > > 32);
> > > > > > > + asf->dar[0].den = (int)get_value(s->pb,
> > > value_type,
> > > > > 32);
> > > > > > > else
> > > > > > > get_tag(s, name, value_type, value_len, 32);
> > > > > > > }
> > > > > > > @@ -630,11 +630,11 @@ static int
> > > > > asf_read_metadata(AVFormatContext *s, int64_t size)
> > > > > > > i, stream_num, name_len_utf16,
> value_type,
> > > > > value_len, name);
> > > > > > >
> > > > > > > if (!strcmp(name, "AspectRatioX")){
> > > > > > > - int aspect_x = get_value(s->pb, value_type,
> 16);
> > > > > > > + int aspect_x = (int)get_value(s->pb,
> value_type,
> > > > > 16);
> > > > > > > if(stream_num < 128)
> > > > > > > asf->dar[stream_num].num = aspect_x;
> > > > > > > } else if(!strcmp(name, "AspectRatioY")){
> > > > > > > - int aspect_y = get_value(s->pb, value_type,
> 16);
> > > > > > > + int aspect_y = (int)get_value(s->pb,
> value_type,
> > > > > 16);
> > > > > > > if(stream_num < 128)
> > > > > > > asf->dar[stream_num].den = aspect_y;
> > > > > > > } else {
> > > > > >
> > > > > > a 64bit/64bit aspect does not work after this, it still
> just
> > > > > silently
> > > > > > truncates the value to 32bit, the truncation just happens
> later
> > > > >
> > > > > forgot: see av_reduce()
> > > >
> > > > These lines are not about making any change. The get_value()
> calls
> > > > for AspectRatioX/Y are returning 16bit values, before and
> after.
> > > >
> > > > I added the explicit cast to (int) because get_value() doesn't
> > > return
> > > > int anymore and as such, to indicate that the cast is
> intentional.
> > > > But the values in this case are limited to 16bit integers
> anyway.
> > >
> > > I dont see what would stop a file that encodes a 64bit aspect
> from
> > > being read as 64bit and then randomly and silently truncated
> > > neither before nor after this patch. The type is read and passed
> > > to get_value() its not checked or i have missed it
> >
> > This is correct. AspectX/Y > INT32_MAX isn't handled correctly,
> > neither before nor after this patch.
> >
> > > I dont understand why adding a explicit cast to int would improve
> > > anything.
> >
> > Before this patch, the return type of get_value() was int, now it's
> > uint64_t. This causes a truncation of the return value that didn't
> > happen before (the truncation happened in get_value()).
> >
>
> > This leads to a compiler/linter warning that hasn't existed before.
> > Adding an explicit cast avoids that and indicates that the
> developer
> > was aware of the truncation. The less warnings a code file
> generates,
> > the better can a developer focus on those that might be actually
> > relevant. That's why I've added the explicit cast.
>
> whats the point of a warning ?
> is it to point to a potential bug ?
> is it a bug in this case ?
> if it is a bug, the warning is correct
> so why would any action that removes the warning without fixing the
> bug be good ?
It marks this as checked and considered to be unlikely to ever occur
in the real world, and attention should be paid to the other warnings.
My experience from many projects is that warnings only receive attention
when they are being curated carefully and the overall count is low.
I acknowledge though, that different philosophies exist for dealing
with these things
> > > Either 64bit should be supported and converted to a supported
> >
> > We can't do this because AVRational's .num and .den are of type
> int.
>
> you can certainly convert it to 32/32bit and id like to see the
> person
> that can see the difference between a 64/64bit aspect and the closest
> 32bit fraction to that. In fact id like to have that display that can
> show the difference
How could the DAR for an image having a 32bit w/h, require fraction
components > 32bit, unless there's some little troll at work in the
muxer that applies an arbitrary factor to num+den?`
It's not impossible though - yes that's true.
> I wish that the demuxer either (approximetaly) supports the values it
> reads
> or produces warnings/errors. Not silenty misinterprets them totally
> One reason is, i prefer to have a warning pointing to weirdness of a
> file instead of having to search for where some wierdness comes from
I 100% agree to that in general! My point was that there are many other
issues in this code that are much more likely to be hit than this one,
but it's not worth to waste more time on that little bit. I'll add the
warnings and we can put a checkmark behind it :-)
Would you want me to make any other changes or should I just fire a
new version?
Thanks again,
softworkz
More information about the ffmpeg-devel
mailing list