On Fri, Oct 01, 2021 at 08:44:04PM +0000, Soft Works wrote:
>
>
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of
> > Michael Niedermayer
> > Sent: Friday, 1 October 2021 19:56
> > To: FFmpeg development discussions and patches <ffmpeg-
> > de...@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-boun...@ffmpeg.org> On Behalf Of
> > > > Michael Niedermayer
> > > > Sent: Thursday, 30 September 2021 14:04
> > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > de...@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 <softwo...@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
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
