diff mbox series

[FFmpeg-devel] avformat/mov: Do not hard fail if bit rate calculation overflows unless in explode mode

Message ID 20211012135040.764167-1-derek.buitenhuis@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/mov: Do not hard fail if bit rate calculation overflows unless in explode mode
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Derek Buitenhuis Oct. 12, 2021, 1:50 p.m. UTC
bit_rate is not a critical field, and we shouln't hard fail if we
can't caluclate it due to a large timebase - it needlessly breaks
valid files.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
---
 libavformat/mov.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Comments

Michael Niedermayer Oct. 12, 2021, 2:43 p.m. UTC | #1
On Tue, Oct 12, 2021 at 02:50:40PM +0100, Derek Buitenhuis wrote:
> bit_rate is not a critical field, and we shouln't hard fail if we
> can't caluclate it due to a large timebase - it needlessly breaks
> valid files.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
>  libavformat/mov.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index d0b8b2595b..99d10c2b33 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -7666,11 +7666,12 @@ static int mov_read_header(AVFormatContext *s)
>              MOVStreamContext *sc = st->priv_data;
>              if (st->duration > 0) {
>                  if (sc->data_size > INT64_MAX / sc->time_scale / 8) {
> -                    av_log(s, AV_LOG_ERROR, "Overflow during bit rate calculation %"PRId64" * 8 * %d\n",
> +                    av_log(s, AV_LOG_WARNING, "Overflow during bit rate calculation %"PRId64" * 8 * %d\n",
>                             sc->data_size, sc->time_scale);
> -                    return AVERROR_INVALIDDATA;
> -                }
> -                st->codecpar->bit_rate = sc->data_size * 8 * sc->time_scale / st->duration;
> +                    if (s->error_recognition & AV_EF_EXPLODE)
> +                        return AVERROR_INVALIDDATA;
> +                } else
> +                    st->codecpar->bit_rate = sc->data_size * 8 * sc->time_scale / st->duration;
>              }

this should be using av_rescale() i think

thx


[...]
Derek Buitenhuis Oct. 12, 2021, 3:17 p.m. UTC | #2
On 10/12/2021 3:43 PM, Michael Niedermayer wrote:
> this should be using av_rescale() i think

That seems unrelated to this patch - but I can send a second patch
that does that, if you want.

Is your intent something like this;

    if (sc->data_size > INT64_MAX / 8) {
        av_log(s, AV_LOG_WARNING, "Overflow during bit rate calculation %"PRId64" * 8 > INT64_MAX\n",
               sc->data_size);
        if (s->error_recognition & AV_EF_EXPLODE)
            return AVERROR_INVALIDDATA;
    } else
        st->codecpar->bit_rate = av_rescale(sc->data_size * 8, c->time_scale, st->duration);

- Derek
Michael Niedermayer Oct. 12, 2021, 7:18 p.m. UTC | #3
On Tue, Oct 12, 2021 at 04:17:31PM +0100, Derek Buitenhuis wrote:
> On 10/12/2021 3:43 PM, Michael Niedermayer wrote:
> > this should be using av_rescale() i think
> 
> That seems unrelated to this patch - but I can send a second patch
> that does that, if you want.
> 
> Is your intent something like this;
> 
>     if (sc->data_size > INT64_MAX / 8) {
>         av_log(s, AV_LOG_WARNING, "Overflow during bit rate calculation %"PRId64" * 8 > INT64_MAX\n",
>                sc->data_size);
>         if (s->error_recognition & AV_EF_EXPLODE)
>             return AVERROR_INVALIDDATA;
>     } else
>         st->codecpar->bit_rate = av_rescale(sc->data_size * 8, c->time_scale, st->duration);

i was thinking of something like
st->codecpar->bit_rate = av_rescale(sc->data_size, c->time_scale * 8LL, st->duration);

because i thought that would fix the overflow
but i didnt look beyond the code in this patch
this may still require some check so the value fits in bit_rate, i didnt
investigate that

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index d0b8b2595b..99d10c2b33 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -7666,11 +7666,12 @@  static int mov_read_header(AVFormatContext *s)
             MOVStreamContext *sc = st->priv_data;
             if (st->duration > 0) {
                 if (sc->data_size > INT64_MAX / sc->time_scale / 8) {
-                    av_log(s, AV_LOG_ERROR, "Overflow during bit rate calculation %"PRId64" * 8 * %d\n",
+                    av_log(s, AV_LOG_WARNING, "Overflow during bit rate calculation %"PRId64" * 8 * %d\n",
                            sc->data_size, sc->time_scale);
-                    return AVERROR_INVALIDDATA;
-                }
-                st->codecpar->bit_rate = sc->data_size * 8 * sc->time_scale / st->duration;
+                    if (s->error_recognition & AV_EF_EXPLODE)
+                        return AVERROR_INVALIDDATA;
+                } else
+                    st->codecpar->bit_rate = sc->data_size * 8 * sc->time_scale / st->duration;
             }
         }
     }
@@ -7681,12 +7682,13 @@  static int mov_read_header(AVFormatContext *s)
             MOVStreamContext *sc = st->priv_data;
             if (sc->duration_for_fps > 0) {
                 if (sc->data_size > INT64_MAX / sc->time_scale / 8) {
-                    av_log(s, AV_LOG_ERROR, "Overflow during bit rate calculation %"PRId64" * 8 * %d\n",
+                    av_log(s, AV_LOG_WARNING, "Overflow during bit rate calculation %"PRId64" * 8 * %d\n",
                            sc->data_size, sc->time_scale);
-                    return AVERROR_INVALIDDATA;
-                }
-                st->codecpar->bit_rate = sc->data_size * 8 * sc->time_scale /
-                    sc->duration_for_fps;
+                    if (s->error_recognition & AV_EF_EXPLODE)
+                        return AVERROR_INVALIDDATA;
+                } else
+                    st->codecpar->bit_rate = sc->data_size * 8 * sc->time_scale /
+                        sc->duration_for_fps;
             }
         }
     }