[FFmpeg-devel] [PATCH v5 2/7] libavformat/asfdec: Fix get_value return type
Michael Niedermayer
michael at niedermayer.cc
Sat Oct 2 14:46:22 EEST 2021
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 ?
>
> > 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
>
> > ratio or it should produce some warning/error not silent truncation
>
> The width and height fields in the ASF Specification are coded as
> 32bit (see Advanced Systems Format Specification, Revision 1.20.03,
> pages 82 and 85), so it would require a really weird encoder
> that emits non-reduced AspectX/Y ration component values as QWORDS.
>
> If we take such unlikely cases as a general measure, there would
> be quite a number of additional checks that we would need to add.
> This patchset deals with incorrect assumptions about realistic cases,
> I can add checks for the less realistic ones as well, if you wish.
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
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20211002/595d3a24/attachment.sig>
More information about the ffmpeg-devel
mailing list