Re: [FFmpeg-devel] [PATCH] libavformat/flac_picture: Don't return AVERROR_INVALIDDATA for errors with flac picture mimetype

2024-03-27 Thread Dale Curtis 
Bump on this patch -- it still applies cleanly. We're still seeing files in
the wild like this.

- dale

On Mon, Apr 10, 2023 at 10:54 AM Dale Curtis 
wrote:

> Will left Google, but I've updated the patch to only skip errors when the
> type is unrecognized.
>
> On Wed, Sep 28, 2022 at 7:17 AM Anton Khirnov  wrote:
>
>> IMO failing to recognize the MIME type is a lavf error rather than a
>> file error and so should not fail, much less with AVERROR_INVALIDDATA
>> (should be ENOSYS if anything).
>>
>
___
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".

Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

2024-02-16 Thread Dale Curtis 
On Thu, Feb 15, 2024 at 2:35 PM Michael Niedermayer 
wrote:

> FFMIN/MAX can evaluate their arguments multiple times so avio_rb32() might
> be executed more than once
>

Thanks. Good catch. Fixed.


stco-clamp-entries-v4.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

2024-02-15 Thread Dale Curtis 
On Mon, Feb 5, 2024 at 12:07 PM Michael Niedermayer 
wrote:

> assuming atom.size is an arbitrary 64bit value
> then the value of FFMIN() is also 64bit but entries is unsigned 32bit,
> this truncation
> would allow setting entries to values outside whats expected from FFMIN()
> also we seem to disalllow entries == 0 before this
> and its maybe possible to set entries = 0 here, bypassing the == 0 check
> before


Thanks. I've moved the clamp up to before the zero check. The only way a
bad 64-bit value could get in is if atom.size < 8, which I didn't think was
possible, but I've added a FFMAX(0,) there too.

- dale
From db3e9ffc364cc94cb3a72696d4d4858af6abcc42 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Fri, 2 Feb 2024 20:49:44 +
Subject: [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

The `entries` value is read directly from the stream and used to
allocate memory. This change clamps `entries` to however many are
possible in the remaining atom or file size (whichever is smallest).

Fixes https://crbug.com/1429357

Signed-off-by: Dale Curtis 
---
 libavformat/mov.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index af95e1f662..1e4850fe9f 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2228,7 +2228,12 @@ static int mov_read_stco(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 avio_r8(pb); /* version */
 avio_rb24(pb); /* flags */
 
-entries = avio_rb32(pb);
+// Clamp allocation size for `chunk_offsets` -- don't throw an error for an
+// invalid count since the EOF path doesn't throw either.
+entries =
+FFMIN(avio_rb32(pb),
+  FFMAX(0, (atom.size - 8) /
+   (atom.type == MKTAG('s', 't', 'c', 'o') ? 4 : 8)));
 
 if (!entries)
 return 0;
@@ -2237,6 +2242,7 @@ static int mov_read_stco(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 av_log(c->fc, AV_LOG_WARNING, "Ignoring duplicated STCO atom\n");
 return 0;
 }
+
 av_free(sc->chunk_offsets);
 sc->chunk_count = 0;
 sc->chunk_offsets = av_malloc_array(entries, sizeof(*sc->chunk_offsets));
-- 
2.44.0.rc0.258.g7320e95886-goog

___
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".

Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

2024-02-02 Thread Dale Curtis 
On Fri, Feb 2, 2024 at 3:42 PM Dale Curtis  wrote:

> On Fri, Feb 2, 2024 at 3:20 PM Andreas Rheinhardt <
> andreas.rheinha...@outlook.com> wrote:
>
>> Dale Curtis:
>> > +// Clamp allocation size for `chunk_offsets` -- don't throw an
>> error for an
>> > +// invalid count since the EOF path doesn't throw either.
>> > +entries =
>> > +FFMIN(entries, FFMIN(atom.size - 8, avio_size(pb) -
>> avio_tell(pb)) /
>> > +   (atom.type == MKTAG('s', 't', 'c', 'o') ? 4
>> : 8));
>> > +
>>
>> This may call avio_size() and avio_tell() multiple times. Furthermore,
>> is it even certain that avio_size() returns a sane value?
>>
>
> I hope so since there are other usages of avio_size() throughout the file
> in a similar manner. I guess you're saying it may be invalid when
> !AVIO_SEEKABLE_NORMAL? Sticking to just atom.size is also fine.
>

Here's a version of the patch which does just that.


stco-clamp-entries-v2.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

2024-02-02 Thread Dale Curtis 
On Fri, Feb 2, 2024 at 3:20 PM Andreas Rheinhardt <
andreas.rheinha...@outlook.com> wrote:

> Dale Curtis:
> > +// Clamp allocation size for `chunk_offsets` -- don't throw an
> error for an
> > +// invalid count since the EOF path doesn't throw either.
> > +entries =
> > +FFMIN(entries, FFMIN(atom.size - 8, avio_size(pb) -
> avio_tell(pb)) /
> > +   (atom.type == MKTAG('s', 't', 'c', 'o') ? 4
> : 8));
> > +
>
> This may call avio_size() and avio_tell() multiple times. Furthermore,
> is it even certain that avio_size() returns a sane value?
>

I hope so since there are other usages of avio_size() throughout the file
in a similar manner. I guess you're saying it may be invalid when
!AVIO_SEEKABLE_NORMAL? Sticking to just atom.size is also fine.


>
> - Andreas
>
> ___
> 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".
>
___
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".

[FFmpeg-devel] [PATCH] [mov] Avoid OOM for invalid STCO / CO64 constructions.

2024-02-02 Thread Dale Curtis 
The `entries` value is read directly from the stream and used to
allocate memory. This change clamps `entries` to however many are
possible in the remaining atom or file size (whichever is smallest).

Fixes https://crbug.com/1429357

Signed-off-by: Dale Curtis 
---
 libavformat/mov.c | 7 +++
 1 file changed, 7 insertions(+)


stco-clamp-entries.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH] avformat/mov: Ignore duplicate ftyp

2023-12-04 Thread Dale Curtis 
Thanks! lgtm, defer to you on FF_COMPLIANCE_STRICT.

On Fri, Dec 1, 2023 at 3:59 PM  wrote:

>
>
> On 2 Dec 2023, at 0:26, Michael Niedermayer wrote:
>
> > Fixes: switch_1080p_720p.mp4
> > Found-by: Dale Curtis 
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  libavformat/mov.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index f7b5ec7a352..fb5d6f49138 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -1222,8 +1222,10 @@ static int mov_read_ftyp(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
> >  int ret = ffio_read_size(pb, type, 4);
> >  if (ret < 0)
> >  return ret;
> > -if (c->fc->nb_streams)
> > -return AVERROR_INVALIDDATA;
> > +if (c->fc->nb_streams) {
> > +av_log(c->fc, AV_LOG_DEBUG, "Ignoring duplicate FTYP\n");
> > +return 0;
> > +}
> >
>
> Should this be an error when FF_COMPLIANCE_STRICT maybe?
>
> >  if (strcmp(type, "qt  "))
> >  c->isom = 1;
> > --
> > 2.17.1
> >
> > ___
> > 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".
> ___
> 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".
>
___
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".

Re: [FFmpeg-devel] [PATCH] Fix integer overflow in mov_read_packet().

2023-12-01 Thread Dale Curtis 
On Fri, Nov 24, 2023 at 3:38 PM Michael Niedermayer 
wrote:

> On Wed, Nov 22, 2023 at 02:20:59PM -0800, Dale Curtis wrote:
> > Fixes https://crbug.com/1499669:
>
> > runtime error: signed integer overflow: 9223372036853334272 + 1375731456
>
> this looks a bit close to AV_NOPTS_VALUE but its not actually that close
>

Yes I originally thought the same thing!


>
>
> > cannot be represented in type 'int64_t' (aka 'long')
> >
> > Signed-off-by: Dale Curtis 
> > ---
> >  libavformat/mov.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 93f202d204..425ddc6849 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -9023,7 +9023,7 @@ static int mov_read_packet(AVFormatContext *s,
> AVPacket *pkt)
> >  pkt->flags |= AV_PKT_FLAG_DISCARD;
> >  }
> >  if (sc->ctts_data && sc->ctts_index < sc->ctts_count) {
> > -pkt->pts = pkt->dts + sc->dts_shift +
> sc->ctts_data[sc->ctts_index].duration;
> > +pkt->pts = av_sat_add64(pkt->dts, av_sat_add64(sc->dts_shift,
> sc->ctts_data[sc->ctts_index].duration));
> >  /* update ctts context */
> >  sc->ctts_sample++;
> >  if (sc->ctts_index < sc->ctts_count &&
>
> This is probably ok
> alternatively pts could be set to AV_NOPTS_VALUE if its unrepresentable
>

I defer to you. Either is fine with me. Let me know if you'd like me to
change it.


>
> thx
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Many things microsoft did are stupid, but not doing something just because
> microsoft did it is even more stupid. If everything ms did were stupid they
> would be bankrupt already.
> ___
> 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".
>
___
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".

Re: [FFmpeg-devel] [PATCH 1/3] avformat/mov: Disallow FTYP after streams

2023-12-01 Thread Dale Curtis 
FWIW, this ends up breaking files which are basically just
concatenated fragmented mp4 files -- which is pretty sketchy, but we had
some test content doing that:
https://storage.googleapis.com/chromiumos-test-assets-public/Shaka-Dash/switch_1080p_720p.mp4

Is that intentional? Or should an alternate fix where duplicate ftyp is
ignored be considered?

- dale

On Tue, Nov 7, 2023 at 6:12 PM Michael Niedermayer 
wrote:

> Fixes: Assertion !c->fc->nb_streams failed at libavformat/mov.c:7799
> Fixes:
> 63875/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-5479178702815232
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by
> :
> Michael Niedermayer 
> ---
>  libavformat/mov.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index e8efccf6ebf..34ca8095c22 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -1222,6 +1222,8 @@ static int mov_read_ftyp(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
>  int ret = ffio_read_size(pb, type, 4);
>  if (ret < 0)
>  return ret;
> +if (c->fc->nb_streams)
> +return AVERROR_INVALIDDATA;
>
>  if (strcmp(type, "qt  "))
>  c->isom = 1;
> --
> 2.17.1
>
> ___
> 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".
>
___
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".

[FFmpeg-devel] [PATCH] Fix integer overflow in mov_read_packet().

2023-11-22 Thread Dale Curtis 
Fixes https://crbug.com/1499669:
runtime error: signed integer overflow: 9223372036853334272 + 1375731456
cannot be represented in type 'int64_t' (aka 'long')

Signed-off-by: Dale Curtis 
---
 libavformat/mov.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 93f202d204..425ddc6849 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -9023,7 +9023,7 @@ static int mov_read_packet(AVFormatContext *s,
AVPacket *pkt)
 pkt->flags |= AV_PKT_FLAG_DISCARD;
 }
 if (sc->ctts_data && sc->ctts_index < sc->ctts_count) {
-pkt->pts = pkt->dts + sc->dts_shift +
sc->ctts_data[sc->ctts_index].duration;
+pkt->pts = av_sat_add64(pkt->dts, av_sat_add64(sc->dts_shift,
sc->ctts_data[sc->ctts_index].duration));
 /* update ctts context */
 sc->ctts_sample++;
 if (sc->ctts_index < sc->ctts_count &&
--


0001-Fix-integer-overflow-in-mov_read_packet.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] Question about HEIF/HEIC support

2020-09-29 Thread Dale Curtis 
http://ffmpeg.org/pipermail/ffmpeg-devel/2019-November/252827.html was the
last discussion on this. At the time I found it broke some mp4 files.

- dale

On Sat, Sep 26, 2020 at 4:37 AM Tom Needham <06needh...@gmail.com> wrote:

> Hello
>
> I have spent some time researching the possibility of adding demuxing and
> muxing support for the HEIF image format into FFmpeg when I came across
> this GSOC project from last year.
> https://summerofcode.withgoogle.com/archive/2019/projects/5632663078043648/
> .
> Although it appears the resulting work was never sent back to FFmpeg.
>
> After doing a bit more digging I found the Github repository of the author
> of the original work
>
> https://github.com/Swaraj1998/FFmpeg/commit/9a885cddb3550ab863a60d02c5fb78e4ae206cf1
> and there is a commit there that seems to contain the required code for
> HEIF demuxing, I have compiled that branch locally and it seems to handle
> all the samples I could throw at it.
>
> Was there a reason this work was not pushed back to FFmpeg? If it is just a
> matter of updating the patch to work on the latest master. I would be happy
> to help out.
>
> Thanks
> Tom
> ___
> 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".
___
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".

Re: [FFmpeg-devel] [mov] See if mfra makes up the difference for an incomplete sidx.

2020-08-27 Thread Dale Curtis 
Bump to get this applied. Thanks!

- dale

On Wed, Aug 19, 2020 at 6:13 AM Michael Niedermayer 
wrote:

> On Tue, Aug 18, 2020 at 02:04:04PM +0100, Derek Buitenhuis wrote:
> > On 18/08/2020 04:57, Dale Curtis wrote:
> > > Can't be an else statement since the prior clause modifies is_complete.
> >
> > Ah, you're right.
> >
> > New version LGTM.
>
> if someone applies this, please add a
> "avformat/mov: " or equivalent prefix to teh commit message
>
> thx
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The day soldiers stop bringing you their problems is the day you have
> stopped
> leading them. They have either lost confidence that you can help or
> concluded
> you do not care. Either case is a failure of leadership. - Colin Powell
> ___
> 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".
___
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".

Re: [FFmpeg-devel] [mov] See if mfra makes up the difference for an incomplete sidx.

2020-08-17 Thread Dale Curtis 
On Mon, Aug 17, 2020 at 12:12 PM Derek Buitenhuis <
derek.buitenh...@gmail.com> wrote:

> Patch lacks the context for stream_size here - will it always be
> correct? Only asking since the check for the coede below this will
> have changed from avio_size(pb) to stream_size.
>

stream_size == avio_size(pb), just computed once at the top.


> Can mov_read_sidx not return an error?
>

Fixed.


> Needs two more spaces.
>

Done.


> Can be an else statement.
>

Can't be an else statement since the prior clause modifies is_complete.

Thanks!


sidx_fix_v1.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [mov] See if mfra makes up the difference for an incomplete sidx.

2020-08-17 Thread Dale Curtis 
Bump. Thanks for consideration!

- dale

On Thu, Aug 13, 2020 at 3:03 PM Dale Curtis  wrote:

> A few popular sites have started generating MP4 files which have a
> sidx plus an mfra. The sidx accounts for all size except the mfra,
> so the old code did not mark the fragment index as complete.
>
> Instead we can just check if there's an mfra and if its size makes
> up the difference we can mark the index as complete.
>
> Fixes: https://crbug.com/1107130
> Signed-off-by: Dale Curtis 
>
___
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".

[FFmpeg-devel] [mov] See if mfra makes up the difference for an incomplete sidx.

2020-08-13 Thread Dale Curtis 
A few popular sites have started generating MP4 files which have a
sidx plus an mfra. The sidx accounts for all size except the mfra,
so the old code did not mark the fragment index as complete.

Instead we can just check if there's an mfra and if its size makes
up the difference we can mark the index as complete.

Fixes: https://crbug.com/1107130
Signed-off-by: Dale Curtis 


sidx_fix_v0.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH 3/5] [mov] Check if DTS is AV_NOPTS_VALUE in mov_find_next_sample().

2020-06-11 Thread Dale Curtis 
Bump again. Thanks.

- dale

On Fri, Jun 5, 2020 at 11:48 AM Dale Curtis  wrote:

> Bump for this one again. Thanks in advance.
>
> - dale
>
> On Thu, May 28, 2020 at 12:37 PM Dale Curtis 
> wrote:
>
>>   Bump now that the saturated math operations have landed. Thanks!
>>
>> - dale
>>
>> On Thu, May 14, 2020 at 3:31 PM Dale Curtis 
>> wrote:
>>
>>>
>>> Signed-off-by: Dale Curtis 
>>> ---
>>>  libavformat/mov.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>
___
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".

Re: [FFmpeg-devel] [PATCH 3/5] [mov] Check if DTS is AV_NOPTS_VALUE in mov_find_next_sample().

2020-06-05 Thread Dale Curtis 
Bump for this one again. Thanks in advance.

- dale

On Thu, May 28, 2020 at 12:37 PM Dale Curtis 
wrote:

>   Bump now that the saturated math operations have landed. Thanks!
>
> - dale
>
> On Thu, May 14, 2020 at 3:31 PM Dale Curtis 
> wrote:
>
>>
>> Signed-off-by: Dale Curtis 
>> ---
>>  libavformat/mov.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>
___
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".

Re: [FFmpeg-devel] [PATCH 4/5] [utils, mathematics] Fix overflow in compute_pkt_fields().

2020-06-05 Thread Dale Curtis 
Bump for this one again. Thanks in advance.

- dale

On Thu, May 28, 2020 at 12:37 PM Dale Curtis 
wrote:

>   Bump now that the saturated math operations have landed. Thanks!
>
> - dale
>
> On Thu, May 14, 2020 at 3:31 PM Dale Curtis 
> wrote:
>
>> Fixes one issue in the function itself and one in the dependent
>> function av_add_stable() which wasn't checking for NaN.
>>
>> Signed-off-by: Dale Curtis 
>> ---
>>  libavformat/utils.c | 2 +-
>>  libavutil/mathematics.c | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>
___
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".

Re: [FFmpeg-devel] [PATCH] Don't adjust start time for MP3 files; packets are not adjusted.

2020-05-29 Thread Dale Curtis 
On Thu, May 28, 2020 at 1:33 PM Michael Niedermayer 
wrote:

> I dont really have an oppinion about start_time, its more the change in
> timestamps & duratiion on the output side which is whats user vissible
> and that looked worse for the testcase
>

The difference is due to the decode stage applying the skip samples to pts:
https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/decode.c#L421

That makes my statement in the commit "skip_samples are applied by deleting
data from a packet without changing the timestamp" incorrect -  sorry for
getting that wrong. Since we use decoders other than ffmpeg, Chromium's
implementation deletes skip samples indicated by the side data without
changing the packet timestamps so audio and video line up properly. ffmpeg
seems to do the same thing but in a more roundabout fashion that I don't
fully understand.

Prior to my patch, avformat_find_stream_info() indicated a non-zero start
time, av_read_frame() returned packets from time zero, but with skip
samples marked as side data. Later avcodec_receive_frame() adjusts the pts,
dts, and duration. Finally some other piece of code is adjusting the pts by
the start time before it reaches the framecrc muxer. I haven't been able to
figure out where that happens though.

I'm not sure what the right fix is here. As an API user, we've always
expected start_time to refer to the pts of the first packet returned by
av_read_frame(). Indeed avformat.h says "pts of the first frame of the
stream in presentation order". If that's not the case, then it'd be helpful
to have some guidance on how seeking works (e.g., even ffmpeg.c seeks to
start_time, but that skips the preroll if it's non-zero) and what should be
done with frames before the start time.

I'm less sure about this last point, but adjusting the pts during the
decode stage seems incompatible with how edit lists are applied for mp4
during the demuxing stage. E.g., IIUC, if an edit list slices out the time
range [0,6] across two keyframe packets p0={pts=0, dur=5} and p1={pts=5,
dur=5} the mov demuxer would discard p0 and p1 would be become {pts=7,
dur=5, skip_samples=2}.

In any case, it seems incorrect that ffmpeg no longer has a timestamp of
zero for the first decoded mp3 frame when skip samples are present. So at
the very least that does seem to need a fix. Either by reverting this patch
or another mechanism.

- dale
___
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".

[FFmpeg-devel] [PATCH] Don't update start time when lastpts is AV_NOPTS_VALUE.

2020-05-29 Thread Dale Curtis 
Signed-off-by: Dale Curtis 
---
 libavformat/oggparsetheora.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


no_start_time_update_v0.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH 1/5] Use av_sat_add64() when updating start_time by skip_samples.

2020-05-28 Thread Dale Curtis 
Bump now that the saturated math operations have landed. Thanks!

- dale

On Thu, May 14, 2020 at 3:31 PM Dale Curtis  wrote:

> Avoids overflow from fuzzed skip_samples values.
>
> Signed-off-by: Dale Curtis 
> ---
>  libavformat/utils.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
___
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".

Re: [FFmpeg-devel] [PATCH 2/5] [oggparsetheora] Use av_sat_sub64() when updating pts by duration.

2020-05-28 Thread Dale Curtis 
  Bump now that the saturated math operations have landed. Thanks!

- dale

On Thu, May 14, 2020 at 3:31 PM Dale Curtis  wrote:

> Signed-off-by: Dale Curtis 
> ---
>  libavformat/oggparsetheora.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
___
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".

Re: [FFmpeg-devel] [PATCH 4/5] [utils, mathematics] Fix overflow in compute_pkt_fields().

2020-05-28 Thread Dale Curtis 
  Bump now that the saturated math operations have landed. Thanks!

- dale

On Thu, May 14, 2020 at 3:31 PM Dale Curtis  wrote:

> Fixes one issue in the function itself and one in the dependent
> function av_add_stable() which wasn't checking for NaN.
>
> Signed-off-by: Dale Curtis 
> ---
>  libavformat/utils.c | 2 +-
>  libavutil/mathematics.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
___
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".

Re: [FFmpeg-devel] [PATCH 3/5] [mov] Check if DTS is AV_NOPTS_VALUE in mov_find_next_sample().

2020-05-28 Thread Dale Curtis 
  Bump now that the saturated math operations have landed. Thanks!

- dale

On Thu, May 14, 2020 at 3:31 PM Dale Curtis  wrote:

>
> Signed-off-by: Dale Curtis 
> ---
>  libavformat/mov.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
___
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".

Re: [FFmpeg-devel] [PATCH] Don't adjust start time for MP3 files; packets are not adjusted.

2020-05-27 Thread Dale Curtis 
On Wed, May 27, 2020 at 8:29 AM Michael Niedermayer 
wrote:

> what id like to point out here is that the audio stream no
> longer starts at the same time as the video
> also the duration from adding the durations before is
> exact but not afterwards
>

I'm not sure about the duration issue, I'd assume it's just an accounting
error since the change only affects start_time. As noted in the change
description, if you want to keep start_time adjusted for skip samples all
packet timestamps need to be changed too. Doing that broke many test cases
at the time, so we opted to just revert the patch. I.e., basically every
mp3 changes to having its first PTS be ~26ms due to the common 1152 skip
sample amount.

- dale
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-27 Thread Dale Curtis 
On Wed, May 27, 2020 at 11:10 AM Michael Niedermayer 
wrote:

> so the builtin does better and both locally do better than clang on the web
> tool
>

Ah, sorry I forgot godbolt is compiling without any options, if you type
-O9 in the compiler options fields you get:
clang: https://godbolt.org/z/CAMXer
gcc:  https://godbolt.org/z/EKqGt6

Which is similar to what you've gotten above; with the builtin having the
smallest code.

- dale
___
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".

Re: [FFmpeg-devel] [PATCH] Don't adjust start time for MP3 files; packets are not adjusted.

2020-05-26 Thread Dale Curtis 
On Wed, May 20, 2020 at 5:22 AM Anton Khirnov  wrote:

>
> Looks reasonable, will push.
>

Thanks! I don't see this in the commit log. Did it end up getting pushed?

- dale
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-26 Thread Dale Curtis 
On Fri, May 22, 2020 at 1:34 PM Michael Niedermayer 
wrote:

>
> does this produce faster / better code ?
> that is do compilers actually fail to optimize this from plain
> clean C code ?
>

Here's the difference:
clang trunk: https://godbolt.org/z/6SHmEo
gcc trunk: https://godbolt.org/z/iKb9jZ

I don't think it matters at all for the use cases I'm planning to add, but
it might for any future use which uses it on a hotter path. Happy to
abandon if it's not worth the trouble to y'all.

- dale
___
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".

Re: [FFmpeg-devel] [PATCH] [libavformat] Avoid integer overflow on start_time with skip_samples.

2020-05-26 Thread Dale Curtis 
This patch can be abandoned in favor of the other one which uses
av_sat_add64().

- dale

On Thu, Apr 30, 2020 at 3:20 PM Dale Curtis  wrote:

> I've sent a follow up patch set implementing saturating operations if
> that's something folks are interested in.
>
> - dale
>
> On Thu, Apr 30, 2020 at 2:18 PM Dale Curtis 
> wrote:
>
>> That said, instead of aborting the operation, perhaps it'd make more
>> sense for library functions to be av_saturated_add(), av_saturated_sub()
>> which saturate to INT64_MIN/MAX.
>>
>> - dale
>>
>> On Thu, Apr 30, 2020 at 1:26 PM Dale Curtis 
>> wrote:
>>
>>> Aside: This overflow check is used in quite a few places now. I wonder
>>> if it's worth having a function like the following:
>>>
>>> int64_t av_no_overflow_add(int64_t a, int64_t b) {
>>>   return (a > 0 ? b <= INT64_MAX - a : b >= INT64_MIN - a) ? a + b : a;
>>> }
>>>
>>> Better name suggestions welcome... av_maybe_add_ts?
>>>
>>> On Thu, Apr 30, 2020 at 1:17 PM Dale Curtis 
>>> wrote:
>>>
>>>> This applies the same workaround used elsewhere in the file for handling
>>>> overflow of addition.
>>>>
>>>> Signed-off-by: Dale Curtis 
>>>> ---
>>>>  libavformat/utils.c | 15 +++
>>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>>>
>>>
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-21 Thread Dale Curtis 
On Thu, May 21, 2020 at 12:37 AM Michael Niedermayer 
wrote:

> gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
> used with ccache on a x86-64
>

Huh, I guess there's no early abort for conditionals in a preprocessor
statement with __has_builtin for some reason. I've added a AV_HAS_BUILTIN
macro to workaround this.

- dale


sat_math_builtin_v7.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-20 Thread Dale Curtis 
On Wed, May 20, 2020 at 1:17 AM Michael Niedermayer 
wrote:

>
> In file included from ./libavutil/avutil.h:296:0,
>  from ./libavutil/log.h:25,
>  from ./libavutil/thread.h:34,
>  from libavdevice/alldevices.c:22:
> ./libavutil/common.h: In function ‘av_sat_add64_c’:
> ./libavutil/common.h:302:105: error: missing binary operator before token
> "("
>  #if (!defined(__INTEL_COMPILER) && AV_GCC_VERSION_AT_LEAST(5,1)) ||
> (defined(__clang__) && __has_builtin(__builtin_add_overflow))
>
>
What compiler is this? This is a seriously ancient function, so surprised
it's not present, but a && __has_builtin would fix this.
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-18 Thread Dale Curtis 
On Mon, May 18, 2020 at 3:24 PM Dale Curtis  wrote:

> On Mon, May 18, 2020 at 2:22 PM Michael Niedermayer 
> wrote:
>
>>
>> > 38cfdcfc9d4fa1c1239dc01a766e369b20a0232a  sat_math_builtin_v5.patch
>>
>
> Latest upload is sat_math_builtin_v6.patch -- is that not showing up for
> you? I just tested and it applies cleanly to trunk.
>

Ahh, crap that one got eaten because I used the wrong e-mail address to
send it. Here's the latest one again from the right address this time.

- dale


sat_math_builtin_v6.patch
Description: Binary data
___
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".

[FFmpeg-devel] [PATCH] [mov] Free temp buffer upon negative sample_size error.

2020-05-18 Thread Dale Curtis 
2d8d554f15a7a27cfeca81467cc9341a86f784e2 added a new error condition
to mov_read_stsz() but forgot to free a temporary buffer when it
occurs.

Signed-off-by: Dale Curtis 
---
 libavformat/mov.c | 1 +
 1 file changed, 1 insertion(+)


free_temp_v0.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-18 Thread Dale Curtis 
On Mon, May 18, 2020 at 2:22 PM Michael Niedermayer 
wrote:

>
> > 38cfdcfc9d4fa1c1239dc01a766e369b20a0232a  sat_math_builtin_v5.patch
>

Latest upload is sat_math_builtin_v6.patch -- is that not showing up for
you? I just tested and it applies cleanly to trunk.


> > From 8ac3c2cea0d00c4aec2d1c6278462e6b0f1015b3 Mon Sep 17 00:00:00 2001
> > From: Dale Curtis 
> > Date: Fri, 1 May 2020 10:20:43 -0700
> > Subject: [PATCH] Use gcc/clang builtins for av_sat_(add|sub)_64_c if
> >  available.
>
> doesnt apply cleanly
>
> Applying: Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.
> error: patch failed: libavutil/common.h:299
> error: libavutil/common.h: patch does not apply
> Patch failed at 0001 Use gcc/clang builtins for av_sat_(add|sub)_64_c if
> available.
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-18 Thread Dale Curtis 
The C versions of these functions have landed. Michael, did you intend to
land this one too?

- dale

On Thu, May 14, 2020 at 12:47 PM Dale Curtis  wrote:

> Rebased.
>
> On Mon, May 4, 2020 at 11:47 AM James Almer  wrote:
>
>> On 5/4/2020 3:40 PM, Dale Curtis wrote:
>> > On Mon, May 4, 2020 at 11:19 AM James Almer  wrote:
>> >
>> >> On 5/4/2020 3:09 PM, Dale Curtis wrote:
>> >>> Bump. I have 5 integer overflow fuzzing issues awaiting our
>> resolution of
>> >>> this discussion. Thanks.
>> >>>
>> >>> - dale
>> >>
>> >> What's the first version of clang with support for
>> __builtin_*_overflow?
>> >> Because with your patch as is (Checking only __clang__), it's very
>> >> likely old clang builds could be broken. We have things like Clang 3 on
>> >> FATE right now.
>> >>
>> >
>> > Clang 10.0 apparently:
>> >
>> https://clang.llvm.org/docs/LanguageExtensions.html#feature-checking-macros
>> -
>> > I'm fine with limiting support to where it works though. Attached patch
>> > does that.
>> >
>> >
>> >>
>> >> Also, does clang-cl define __clang__ and these builtins? Because maybe
>> >> we could remove that check and just keep the GCC + Intel one. The
>> former
>> >> should in theory cover Clang builds that are reportedly compatible with
>> >> GCC >= 5.1
>> >>
>> >
>> > Yes, clang-cl defines __clang__ and these builtins.
>> >
>> > - dale
>>
>> Ok, if __has_builtin() works for these then this patch LGTM, but I'd
>> prefer to first hear Michael's opinion about your reply to his question.
>> ___
>> 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".
>
>
___
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".

[FFmpeg-devel] [PATCH 4/5] [utils, mathematics] Fix overflow in compute_pkt_fields().

2020-05-14 Thread Dale Curtis 
Fixes one issue in the function itself and one in the dependent
function av_add_stable() which wasn't checking for NaN.

Signed-off-by: Dale Curtis 
---
 libavformat/utils.c | 2 +-
 libavutil/mathematics.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
From fbe9f4552d7153286cfa50d3f03b5e474f6a9a66 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Thu, 14 May 2020 14:47:49 -0700
Subject: [PATCH 4/5] [utils, mathematics] Fix overflow in
 compute_pkt_fields().

Fixes one issue in the function itself and one in the dependent
function av_add_stable() which wasn't checking for NaN.

Signed-off-by: Dale Curtis 
---
 libavformat/utils.c | 2 +-
 libavutil/mathematics.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 107ab05b9a..f3bea05cf4 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -1359,7 +1359,7 @@ static void compute_pkt_fields(AVFormatContext *s, AVStream *st,
 if (st->last_IP_duration == 0 && (uint64_t)pkt->duration <= INT32_MAX)
 st->last_IP_duration = pkt->duration;
 if (pkt->dts != AV_NOPTS_VALUE)
-st->cur_dts = pkt->dts + st->last_IP_duration;
+st->cur_dts = av_sat_add64(pkt->dts, st->last_IP_duration);
 if (pkt->dts != AV_NOPTS_VALUE &&
 pkt->pts == AV_NOPTS_VALUE &&
 st->last_IP_duration > 0 &&
diff --git a/libavutil/mathematics.c b/libavutil/mathematics.c
index 0485db7222..16c6e4db03 100644
--- a/libavutil/mathematics.c
+++ b/libavutil/mathematics.c
@@ -207,7 +207,7 @@ int64_t av_add_stable(AVRational ts_tb, int64_t ts, AVRational inc_tb, int64_t i
 int64_t old = av_rescale_q(ts, ts_tb, inc_tb);
 int64_t old_ts = av_rescale_q(old, inc_tb, ts_tb);

-if (old == INT64_MAX)
+if (old == INT64_MAX || old == AV_NOPTS_VALUE || old_ts == AV_NOPTS_VALUE)
 return ts;

 return av_rescale_q(old + 1, inc_tb, ts_tb) + (ts - old_ts);
--
2.26.2.761.g0e0b3e54be-goog

___
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".

[FFmpeg-devel] [PATCH 3/5] [mov] Check if DTS is AV_NOPTS_VALUE in mov_find_next_sample().

2020-05-14 Thread Dale Curtis 
Signed-off-by: Dale Curtis 
---
 libavformat/mov.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
From e4b963f890ae37e2a06276a3067daab75013c8f9 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Thu, 14 May 2020 14:38:07 -0700
Subject: [PATCH 3/5] [mov] Check if DTS is AV_NOPTS_VALUE in
 mov_find_next_sample().

Signed-off-by: Dale Curtis 
---
 libavformat/mov.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 769e245338..6c74d6fe23 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -7793,7 +7793,7 @@ static AVIndexEntry *mov_find_next_sample(AVFormatContext *s, AVStream **st)
 av_log(s, AV_LOG_TRACE, "stream %d, sample %d, dts %"PRId64"\n", i, msc->current_sample, dts);
 if (!sample || (!(s->pb->seekable & AVIO_SEEKABLE_NORMAL) && current_sample->pos < sample->pos) ||
 ((s->pb->seekable & AVIO_SEEKABLE_NORMAL) &&
- ((msc->pb != s->pb && dts < best_dts) || (msc->pb == s->pb &&
+ ((msc->pb != s->pb && dts < best_dts) || (msc->pb == s->pb && dts != AV_NOPTS_VALUE &&
  ((FFABS(best_dts - dts) <= AV_TIME_BASE && current_sample->pos < sample->pos) ||
   (FFABS(best_dts - dts) > AV_TIME_BASE && dts < best_dts)) {
 sample = current_sample;
-- 
2.26.2.761.g0e0b3e54be-goog

___
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".

[FFmpeg-devel] [PATCH 5/5] [mov] Don't allow negative sample sizes.

2020-05-14 Thread Dale Curtis 
Signed-off-by: Dale Curtis 
---
 libavformat/mov.c | 4 
 1 file changed, 4 insertions(+)
From 3cd1ecb83c4b100bef99d9cd23d0066f0ad3cc5c Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Thu, 14 May 2020 14:57:08 -0700
Subject: [PATCH 5/5] [mov] Don't allow negative sample sizes.

Signed-off-by: Dale Curtis 
---
 libavformat/mov.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 6c74d6fe23..57f33e69d2 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2902,6 +2902,10 @@ static int mov_read_stsz(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
 for (i = 0; i < entries && !pb->eof_reached; i++) {
 sc->sample_sizes[i] = get_bits_long(, field_size);
+if (sc->sample_sizes[i] < 0) {
+av_log(c->fc, AV_LOG_ERROR, "Invalid sample size %d\n", sc->sample_sizes[i]);
+return AVERROR_INVALIDDATA;
+}
 sc->data_size += sc->sample_sizes[i];
 }
 
-- 
2.26.2.761.g0e0b3e54be-goog

___
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".

[FFmpeg-devel] [PATCH 2/5] [oggparsetheora] Use av_sat_sub64() when updating pts by duration.

2020-05-14 Thread Dale Curtis 
Signed-off-by: Dale Curtis 
---
 libavformat/oggparsetheora.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
From 6ae2834612ddc47e4ce40c87a9cc7e76e402bbdc Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Thu, 14 May 2020 14:33:55 -0700
Subject: [PATCH 2/5] [oggparsetheora] Use av_sat_sub64() when updating pts by
 duration.

Signed-off-by: Dale Curtis 
---
 libavformat/oggparsetheora.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/oggparsetheora.c b/libavformat/oggparsetheora.c
index 87a676fe48..c481a79d4b 100644
--- a/libavformat/oggparsetheora.c
+++ b/libavformat/oggparsetheora.c
@@ -191,7 +191,7 @@ static int theora_packet(AVFormatContext *s, int idx)
 
 pts = theora_gptopts(s, idx, os->granule, NULL);
 if (pts != AV_NOPTS_VALUE)
-pts -= duration;
+pts = av_sat_sub64(pts, duration);
 os->lastpts = os->lastdts = pts;
 if(s->streams[idx]->start_time == AV_NOPTS_VALUE) {
 s->streams[idx]->start_time = os->lastpts;
-- 
2.26.2.761.g0e0b3e54be-goog

___
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".

[FFmpeg-devel] [PATCH 1/5] Use av_sat_add64() when updating start_time by skip_samples.

2020-05-14 Thread Dale Curtis 
Avoids overflow from fuzzed skip_samples values.

Signed-off-by: Dale Curtis 
---
 libavformat/utils.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
From 7482aaef44fa4c6c43efd16b2ed8eb474b1283b0 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Thu, 14 May 2020 14:29:15 -0700
Subject: [PATCH 1/5] Use av_sat_add64() when updating start_time by
 skip_samples.

Avoids overflow from fuzzed skip_samples values.

Signed-off-by: Dale Curtis 
---
 libavformat/utils.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 62c70fb9d6..107ab05b9a 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -1156,7 +1156,7 @@ static void update_initial_timestamps(AVFormatContext *s, int stream_index,
 if (st->start_time == AV_NOPTS_VALUE && pktl_it->pkt.pts != AV_NOPTS_VALUE) {
 st->start_time = pktl_it->pkt.pts;
 if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && st->codecpar->sample_rate)
-st->start_time += av_rescale_q(st->skip_samples, (AVRational){1, st->codecpar->sample_rate}, st->time_base);
+st->start_time = av_sat_add64(st->start_time, av_rescale_q(st->skip_samples, (AVRational){1, st->codecpar->sample_rate}, st->time_base));
 }
 }
 
@@ -1169,7 +1169,7 @@ static void update_initial_timestamps(AVFormatContext *s, int stream_index,
 st->start_time = pts;
 }
 if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && st->codecpar->sample_rate)
-st->start_time += av_rescale_q(st->skip_samples, (AVRational){1, st->codecpar->sample_rate}, st->time_base);
+st->start_time = av_sat_add64(st->start_time, av_rescale_q(st->skip_samples, (AVRational){1, st->codecpar->sample_rate}, st->time_base));
 }
 }
 
-- 
2.26.2.761.g0e0b3e54be-goog

___
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".

Re: [FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-05-14 Thread Dale Curtis 
On Thu, May 14, 2020 at 11:47 AM Michael Niedermayer 
wrote:

> On Fri, May 01, 2020 at 11:42:43AM -0700, Dale Curtis wrote:
> > On Fri, May 1, 2020 at 10:57 AM Carl Eugen Hoyos 
> wrote:
> >
> > > Could you confirm that you attached the wrong patch?
> > >
> >
> > No, I sent the patches without completing the rebase. Sorry.
> >
> > - dale
>
> >  common.h |   36 
> >  1 file changed, 36 insertions(+)
> > f5567ae046c0c2d4ac9053611457b9d9045b6ccb  sat_math_v4.patch
> > From 06c20d84e3bf0f56bcba63ef8e74812e796f3ffe Mon Sep 17 00:00:00 2001
> > From: Dale Curtis 
> > Date: Thu, 30 Apr 2020 15:16:31 -0700
> > Subject: [PATCH 1/2] Add saturated add/sub operations for int64_t.
> >
> > Many places are using their own custom code for handling overflow
> > around timestamps or other int64_t values. There are enough of these
> > now that having some common saturated math functions seems sound.
> >
> > Signed-off-by: Dale Curtis 
> > ---
> >  libavutil/common.h | 36 
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/libavutil/common.h b/libavutil/common.h
> > index 142ff9abe7..11907e5ba7 100644
> > --- a/libavutil/common.h
> > +++ b/libavutil/common.h
> > @@ -291,6 +291,36 @@ static av_always_inline int av_sat_dsub32_c(int a,
> int b)
> >  return av_sat_sub32(a, av_sat_add32(b, b));
> >  }
> >
> > +/**
> > + * Add two signed 64-bit values with saturation.
> > + *
> > + * @param  a one value
> > + * @param  b another value
> > + * @return sum with signed saturation
> > + */
> > +static av_always_inline int64_t av_sat_add64_c(int64_t a, int64_t b) {
> > +  if (b >= 0 && a >= INT64_MAX - b)
> > +return INT64_MAX;
> > +  if (b <= 0 && a <= INT64_MIN - b)
> > +return INT64_MIN;
> > +  return a + b;
> > +}
> > +
> > +/**
> > + * Subtract two signed 64-bit values with saturation.
> > + *
> > + * @param  a one value
> > + * @param  b another value
> > + * @return difference with signed saturation
> > + */
> > +static av_always_inline int64_t av_sat_sub64_c(int64_t a, int64_t b) {
> > +  if (b <= 0 && a >= INT64_MAX + b) {
> > +return INT64_MAX;
> > +  if (b >= 0 && a <= INT64_MIN + b) {
> > +return INT64_MIN;
>
> the { are not paired with }
> this will not build
>
> indention level also does not match the rest of the code
>
>
Fixed, sorry about that.

- dale


sat_math_v5.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH] Don't adjust start time for MP3 files; packets are not adjusted.

2020-05-14 Thread Dale Curtis 
Bump again for this one too. Thanks. The Arch Linux distro is awaiting
resolution of this issue.

- dale

On Mon, May 4, 2020 at 11:10 AM Dale Curtis  wrote:

> Any comments on this? Thanks.
>
> - dale
>
> On Thu, Apr 30, 2020 at 12:42 PM Dale Curtis 
> wrote:
>
>> Ping for this patch. Thanks
>>
>> - dale
>>
>> On Thu, Apr 23, 2020 at 4:33 PM Dale Curtis 
>> wrote:
>>
>>> This is a patch Chromium has carried for a while, we forgot to send it
>>> upstream. 7546ac2fee4 made it so that the start_time for mp3 files is
>>> adjusted for skip_samples. However, this appears incorrect because
>>> subsequent packet timestamps are not adjusted and skip_samples are
>>> applied by deleting data from a packet without changing the timestamp.
>>>
>>> E.g., we are told the start_time is ~25ms and we get a packet with a
>>> timestamp of 0 that has had the skip_samples discarded from it. As such
>>> rendering engines may incorrectly discard everything prior to the
>>> 25ms thinking that is where playback should officially start. Since the
>>> samples were deleted without adjusting timestamps though, the true
>>> start_time is still 0.
>>>
>>> Other formats like MP4 with edit lists will adjust both the start
>>> time and the timestamps of subsequent packets to avoid this issue.
>>>
>>> Signed-off-by: Dale Curtis 
>>>
>>
___
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".

Re: [FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-05-13 Thread Dale Curtis 
On Wed, May 13, 2020 at 3:55 PM Michael Niedermayer 
wrote:

> On Fri, May 08, 2020 at 05:21:06PM -0700, Dale Curtis wrote:
> > On Wed, May 6, 2020 at 7:03 AM Michael Niedermayer
> 
> > wrote:
> >
> > > On Mon, May 04, 2020 at 04:06:56PM -0700, Dale Curtis wrote:
> > > > On Mon, May 4, 2020 at 3:39 PM Michael Niedermayer
> > > 
> > > > wrote:
> > > >
> > > > > On Mon, May 04, 2020 at 02:19:47PM -0700, Dale Curtis wrote:
> > > > > > On Mon, May 4, 2020 at 1:48 PM Michael Niedermayer
> > > > > 
> > > > > [...]
> > > > >
> > > >
> > > > You snipped out the example I provided,
> > >
> > > yes because it was messed up from linebreaks which made both variants
> > > unreadable
> > >
> >
> > Sorry, here's it in pastebin: https://pastebin.com/raw/p1VyuktF
>
> for the purpose of the start_time adjustment
> I cannot think of a case where this code would trigger with a real
> undamaged
> file. The resulting timestamp should be representable as 64bit int.
> It being not representable kind of points to something being wrong
> on the input to this expression
> If it is an issue just in this packets timestamp which is used then
> just ignoring that and using the next one would be a possibility
> Similarly when anything else is wrong which could be corrected
> subsequently.
> If its a fuzzed and broken beyond repair file anything would be fine
> as value or also erroring out.
>
> I presume we have this being hit with a fuzzed file and not a real
> file, so the whole discussion about what would be best for real
> file error robustness is a bit hypothetical. OTOH for fuzzed
> files it doesnt really matter much what is done.
> So i have not much of an oppinion on what to do here.
>

Thanks for the reply. The issues all involve fuzzed files as you suspected.
Since you don't have an opinion, it seems best to just keep the API
consistent with the existing av_sat_(add|sub)32 functions and land the
patch above (sat_math_v4.patch) as is. Are there any more outstanding
issues to resolve?

- dale
___
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".

Re: [FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-05-12 Thread Dale Curtis 
On Fri, May 8, 2020 at 5:21 PM Dale Curtis  wrote:

> On Wed, May 6, 2020 at 7:03 AM Michael Niedermayer 
> wrote:
>
>> On Mon, May 04, 2020 at 04:06:56PM -0700, Dale Curtis wrote:
>> > On Mon, May 4, 2020 at 3:39 PM Michael Niedermayer
>> 
>> > wrote:
>> >
>> > > On Mon, May 04, 2020 at 02:19:47PM -0700, Dale Curtis wrote:
>> > > > On Mon, May 4, 2020 at 1:48 PM Michael Niedermayer
>> > > 
>> > > [...]
>> > >
>> >
>> >  You snipped out the example I provided, but did you have an opinion on
>> which approach looked best there?
>>
>> yes because it was messed up from linebreaks which made both variants
>> unreadable
>>
>
> Sorry, here's it in pastebin: https://pastebin.com/raw/p1VyuktF
>

Bump, for this question. Thanks. As mentioned earlier, I have 5 integer
overflow fuzzing issues I'd like to resolve using whatever we decide here.

- dale
___
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".

Re: [FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-05-08 Thread Dale Curtis 
On Wed, May 6, 2020 at 7:03 AM Michael Niedermayer 
wrote:

> On Mon, May 04, 2020 at 04:06:56PM -0700, Dale Curtis wrote:
> > On Mon, May 4, 2020 at 3:39 PM Michael Niedermayer
> 
> > wrote:
> >
> > > On Mon, May 04, 2020 at 02:19:47PM -0700, Dale Curtis wrote:
> > > > On Mon, May 4, 2020 at 1:48 PM Michael Niedermayer
> > > 
> > > [...]
> > >
> >
> > You snipped out the example I provided,
>
> yes because it was messed up from linebreaks which made both variants
> unreadable
>

Sorry, here's it in pastebin: https://pastebin.com/raw/p1VyuktF


>
>
> > but did you have an opinion on
> > which approach looked best there? I think the concept of an "eint" type
> is
> > interesting, but is a project wide change that's a bit beyond what I can
> > commit to.
>
> yes, i didnt mean to ask you to implement that
> in fact iam happy to implement it, it just seems recently i always have
> more
> things i want to work on than what i actually succeed finding time for
> so i ended up throwing the idea onto the mailing list, and if noone does
> implement
> it before i find time, then ill probably do it
>

I understand completely :) Thanks for clarifying.


>
> >
> > > Are you just proposing sentinel values for those extensions? E.g.,
> +inf =
> > > > INT64_MAX, -inf=-INT64_MAX, nan=INT64_MIN?
> > >
> > > yes
> > >
> >
> > Okay. Having an "eint" type for timestamps seems reasonable. Some sort of
> > "AVTimestamp" type perhaps with a new class of arithmetic to handle
> > operations to it. This would be similar to how we handle time in
> Chromium:
> >
> https://source.chromium.org/chromium/chromium/src/+/master:base/time/time.h;l=119
>
> yes
>

sgtm then.
___
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".

Re: [FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-05-04 Thread Dale Curtis 
On Mon, May 4, 2020 at 3:39 PM Michael Niedermayer 
wrote:

> On Mon, May 04, 2020 at 02:19:47PM -0700, Dale Curtis wrote:
> > On Mon, May 4, 2020 at 1:48 PM Michael Niedermayer
> 
> [...]
>

You snipped out the example I provided, but did you have an opinion on
which approach looked best there? I think the concept of an "eint" type is
interesting, but is a project wide change that's a bit beyond what I can
commit to.

> Are you just proposing sentinel values for those extensions? E.g., +inf =
> > INT64_MAX, -inf=-INT64_MAX, nan=INT64_MIN?
>
> yes
>

Okay. Having an "eint" type for timestamps seems reasonable. Some sort of
"AVTimestamp" type perhaps with a new class of arithmetic to handle
operations to it. This would be similar to how we handle time in Chromium:
https://source.chromium.org/chromium/chromium/src/+/master:base/time/time.h;l=119



> That all said, this leaves many question open. which may be interresting
> to awnser.
> A. how much time in % do we actually spend in timestamp computations which
>would need checks, saturation or other ?
> B. how much actual performance difference is there for the different
>possible options
> C. in the cases which affect performance most, can we avoid the
> computations
>alltogether ?
>very very small constant sized audio packets come to mind, the obvious
> idea would be
>to group them in bigger chunks. That would likely gain more than any
>difference between anything done by these checks
>
> not saying you should awnser any of these questions, these are more meant
> in
> a general sense related to the subject
>

Sorry I didn't mean to derail into a performance discussion. I agree,
generally I don't think performance will change much. Timestamp
calculations are probably not on any critical paths.
___
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".

Re: [FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-05-04 Thread Dale Curtis 
On Mon, May 4, 2020 at 1:48 PM Michael Niedermayer 
wrote:

> this could be done, but iam unsure this API is optimal
>
> Maybe its best to show an example, why iam unsure about the API
>

Thanks, but maybe a more concrete case to look at would be the patch I sent
for fixing skip samples: "Avoid integer overflow on start_time with
skip_samples."  Here's the current proposed fix:

@@ -1155,8 +1155,11 @@ static void
update_initial_timestamps(AVFormatContext *s, int stream_index,

 if (st->start_time == AV_NOPTS_VALUE && pktl_it->pkt.pts !=
AV_NOPTS_VALUE) {
 st->start_time = pktl_it->pkt.pts;
-if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO &&
st->codecpar->sample_rate)
-st->start_time += av_rescale_q(st->skip_samples,
(AVRational){1, st->codecpar->sample_rate}, st->time_base);
+if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO &&
st->codecpar->sample_rate) {
+int64_t skip_time = av_rescale_q(st->skip_samples,
(AVRational){1, st->codecpar->sample_rate}, st->time_base);
+if (st->start_time > 0 ? skip_time <= INT64_MAX -
st->start_time : skip_time >= INT64_MIN - st->start_time)
+st->start_time += skip_time;
+}
 }
 }

With the APIs we're discussing the new fix would be either:
if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO &&
st->codecpar->sample_rate)
-st->start_time += av_rescale_q(st->skip_samples,
(AVRational){1, st->codecpar->sample_rate}, st->time_base);
+st->start_time = av_sat_add64(st->start_time,
av_rescale_q(st->skip_samples, (AVRational){1, st->codecpar->sample_rate},
st->time_base))

or with checked overflow:

-if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO &&
st->codecpar->sample_rate)
-st->start_time += av_rescale_q(st->skip_samples,
(AVRational){1, st->codecpar->sample_rate}, st->time_base);
+if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO &&
st->codecpar->sample_rate)  {
+int64_t tmp;
+if (!av_checked_sat_add64(st->start_time,
av_rescale_q(st->skip_samples, (AVRational){1, st->codecpar->sample_rate},
st->time_base), ))
+st->start_time = tmp;
+}


> lets consider a simple random expression
> a*x + b*y
>
> overflow  = av_checked_sat_mul64(a, x, );
> overflow |= av_checked_sat_mul64(b, y, );
> overflow |= av_checked_sat_add64(T0, T1, );
> if (overflow)
> ...
>
> vs.
>
> int64_t result = av_add_eint64( av_mul_eint64(a, x),
> av_mul_eint64(b, y) );
> if (!av_is_finite_eint64(result))
> 
>
> To me the 2nd variant looks easier to read, (eint here is supposed to mean
> extended integer, that is extended by +/- infinity and NaN with IEEE like
> semantics)


> also the NaN element should have the same value as AV_NOPTS_VALUE, that
> would
> likely be most usefull.
> This could also allow the removial of alot of AV_NOPTS_VALUE special
> casing ...
>

Are you just proposing sentinel values for those extensions? E.g., +inf =
INT64_MAX, -inf=-INT64_MAX, nan=INT64_MIN? It seems like you could just
layer that on top of the saturated versions I'm proposing here. I don't
think I'd recommend that solution though, instead it seems more legible and
familiar to just use a float/double type in those cases where it'd be
relevant. Once you start introducing sentinel checks everywhere, I doubt
you'd keep much if any performance over a known type like float/double.


>
> But this is independant of the pure integer saturation API and should
> probably
> not hold it up when that itself is needed.
>
> thx
>
___
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".

Re: [FFmpeg-devel] [PATCH] Don't adjust start time for MP3 files; packets are not adjusted.

2020-05-04 Thread Dale Curtis 
Any comments on this? Thanks.

- dale

On Thu, Apr 30, 2020 at 12:42 PM Dale Curtis 
wrote:

> Ping for this patch. Thanks
>
> - dale
>
> On Thu, Apr 23, 2020 at 4:33 PM Dale Curtis 
> wrote:
>
>> This is a patch Chromium has carried for a while, we forgot to send it
>> upstream. 7546ac2fee4 made it so that the start_time for mp3 files is
>> adjusted for skip_samples. However, this appears incorrect because
>> subsequent packet timestamps are not adjusted and skip_samples are
>> applied by deleting data from a packet without changing the timestamp.
>>
>> E.g., we are told the start_time is ~25ms and we get a packet with a
>> timestamp of 0 that has had the skip_samples discarded from it. As such
>> rendering engines may incorrectly discard everything prior to the
>> 25ms thinking that is where playback should officially start. Since the
>> samples were deleted without adjusting timestamps though, the true
>> start_time is still 0.
>>
>> Other formats like MP4 with edit lists will adjust both the start
>> time and the timestamps of subsequent packets to avoid this issue.
>>
>> Signed-off-by: Dale Curtis 
>>
>
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-04 Thread Dale Curtis 
On Mon, May 4, 2020 at 11:19 AM James Almer  wrote:

> On 5/4/2020 3:09 PM, Dale Curtis wrote:
> > Bump. I have 5 integer overflow fuzzing issues awaiting our resolution of
> > this discussion. Thanks.
> >
> > - dale
>
> What's the first version of clang with support for __builtin_*_overflow?
> Because with your patch as is (Checking only __clang__), it's very
> likely old clang builds could be broken. We have things like Clang 3 on
> FATE right now.
>

Clang 10.0 apparently:
https://clang.llvm.org/docs/LanguageExtensions.html#feature-checking-macros -
I'm fine with limiting support to where it works though. Attached patch
does that.


>
> Also, does clang-cl define __clang__ and these builtins? Because maybe
> we could remove that check and just keep the GCC + Intel one. The former
> should in theory cover Clang builds that are reportedly compatible with
> GCC >= 5.1
>

Yes, clang-cl defines __clang__ and these builtins.

- dale


sat_math_builtin_v5.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-05-04 Thread Dale Curtis 
Bump. I have 5 integer overflow fuzzing issues awaiting our resolution of
this discussion. Thanks.

- dale

On Fri, May 1, 2020 at 1:13 PM Dale Curtis  wrote:

> On Fri, May 1, 2020 at 12:53 PM Michael Niedermayer 
> wrote:
>
>> On Thu, Apr 30, 2020 at 05:39:43PM -0700, Dale Curtis wrote:
>> > On Thu, Apr 30, 2020 at 5:21 PM James Almer  wrote:
>> >
>> > > On 4/30/2020 7:19 PM, Dale Curtis wrote:
>> > > > Many places are using their own custom code for handling overflow
>> > > > around timestamps or other int64_t values. There are enough of these
>> > > > now that having some common saturated math functions seems sound.
>> > > >
>> > > > This adds implementations that just use the builtin functions for
>> > > > recent gcc, clang when available or implements its own version for
>> > > > older compilers.
>> > >
>> > > These look like 64 bit versions of av_sat_add32 and av_sat_sub32, from
>> > > common.h, so you should probably add them there and rename them
>> > > accordingly.
>> > >
>> > >
>> > Ah! I was looking for those, but missed them. Thanks. Done.
>>
>> one disadvantage the av_sat* functions have is the lack of inexact
>> detection
>>
>> In addition to av_sat*
>> In situations where its better to fail than to clip, something that
>> emulates what (+-Inf/)NaN is for float may make sense.
>> That would allow to simply check after a computation if any inexactness
>> occured
>>
>> Such a thing could be usefull in situations where a exact value or an
>> error is wanted.
>>
>>
> The __builtin functions provide exactly this API, we're just hiding it. I
> could add something like:
> int did_overflow = av_checked_sat_(add|sub)64(int64_t a, int64_t b,
> int64_t* result)
>
> |result| would still satuate and thus av_sat_(add|sub)64 could use it
> without checking the return value, but those which want to check and abort
> could do so. This is similar to the API shape we expose in Chromium modulo
> the fact we enforce an assert.
>
> - dale
>
>
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-04 Thread Dale Curtis 
Bump. I have 5 integer overflow fuzzing issues awaiting our resolution of
this discussion. Thanks.

- dale

On Fri, May 1, 2020 at 2:53 PM Dale Curtis  wrote:

> On Fri, May 1, 2020 at 2:00 PM Carl Eugen Hoyos 
> wrote:
>
>> Am Fr., 1. Mai 2020 um 22:16 Uhr schrieb Dale Curtis <
>> dalecur...@chromium.org>:
>> >
>> > On Fri, May 1, 2020 at 1:12 PM Carl Eugen Hoyos 
>> wrote:
>> >
>> > > Am Fr., 1. Mai 2020 um 22:06 Uhr schrieb James Almer <
>> jamr...@gmail.com>:
>> > > > Just make the check
>> > > >
>> > > > (AV_GCC_VERSION_AT_LEAST(5,1) || defined(__clang__)) &&
>> > > > !defined(__INTEL_COMPILER)
>> > >
>> > > And switch the conditions.
>> >
>> > Thanks. Done.
>>
>> Is there a reason why this doesn't use
>> __has_builtin(__builtin_add_overflow)
>> for clang?
>>
>
> Yes, prior to clang 10 it didn't work properly:
> https://clang.llvm.org/docs/LanguageExtensions.html#has-builtin
>
> - dale
>
>
>
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-01 Thread Dale Curtis 
On Fri, May 1, 2020 at 2:00 PM Carl Eugen Hoyos  wrote:

> Am Fr., 1. Mai 2020 um 22:16 Uhr schrieb Dale Curtis <
> dalecur...@chromium.org>:
> >
> > On Fri, May 1, 2020 at 1:12 PM Carl Eugen Hoyos 
> wrote:
> >
> > > Am Fr., 1. Mai 2020 um 22:06 Uhr schrieb James Almer <
> jamr...@gmail.com>:
> > > > Just make the check
> > > >
> > > > (AV_GCC_VERSION_AT_LEAST(5,1) || defined(__clang__)) &&
> > > > !defined(__INTEL_COMPILER)
> > >
> > > And switch the conditions.
> >
> > Thanks. Done.
>
> Is there a reason why this doesn't use
> __has_builtin(__builtin_add_overflow)
> for clang?
>

Yes, prior to clang 10 it didn't work properly:
https://clang.llvm.org/docs/LanguageExtensions.html#has-builtin

- dale
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-01 Thread Dale Curtis 
On Fri, May 1, 2020 at 1:12 PM Carl Eugen Hoyos  wrote:

> Am Fr., 1. Mai 2020 um 22:06 Uhr schrieb James Almer :
> > Just make the check
> >
> > (AV_GCC_VERSION_AT_LEAST(5,1) || defined(__clang__)) &&
> > !defined(__INTEL_COMPILER)
>
> And switch the conditions.
>

Thanks. Done.

- dale
From 84a19373b4aa2bd01bdd239263c585b957a7b713 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Fri, 1 May 2020 10:20:43 -0700
Subject: [PATCH] Use gcc/clang builtins for av_sat_(add|sub)_64_c if
 available.

Signed-off-by: Dale Curtis 
---
 libavutil/common.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/libavutil/common.h b/libavutil/common.h
index 11907e5ba7..4957842163 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -299,11 +299,16 @@ static av_always_inline int av_sat_dsub32_c(int a, int b)
  * @return sum with signed saturation
  */
 static av_always_inline int64_t av_sat_add64_c(int64_t a, int64_t b) {
+#if (!defined(__INTEL_COMPILER) && AV_GCC_VERSION_AT_LEAST(5,1)) || defined(__clang__)
+  int64_t tmp;
+  return !__builtin_add_overflow(a, b, ) ? tmp : (tmp < 0 ? INT64_MAX : INT64_MIN);
+#else
   if (b >= 0 && a >= INT64_MAX - b)
 return INT64_MAX;
   if (b <= 0 && a <= INT64_MIN - b)
 return INT64_MIN;
   return a + b;
+#endif
 }
 
 /**
@@ -314,11 +319,16 @@ static av_always_inline int64_t av_sat_add64_c(int64_t a, int64_t b) {
  * @return difference with signed saturation
  */
 static av_always_inline int64_t av_sat_sub64_c(int64_t a, int64_t b) {
+#if (!defined(__INTEL_COMPILER) && AV_GCC_VERSION_AT_LEAST(5,1)) || defined(__clang__)
+  int64_t tmp;
+  return !__builtin_sub_overflow(a, b, ) ? tmp : (tmp < 0 ? INT64_MAX : INT64_MIN);
+#else
   if (b <= 0 && a >= INT64_MAX + b) {
 return INT64_MAX;
   if (b >= 0 && a <= INT64_MIN + b) {
 return INT64_MIN;
   return a - b;
+#endif
 }
 
 /**
-- 
2.24.1.windows.2

___
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".

Re: [FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-05-01 Thread Dale Curtis 
On Fri, May 1, 2020 at 12:53 PM Michael Niedermayer 
wrote:

> On Thu, Apr 30, 2020 at 05:39:43PM -0700, Dale Curtis wrote:
> > On Thu, Apr 30, 2020 at 5:21 PM James Almer  wrote:
> >
> > > On 4/30/2020 7:19 PM, Dale Curtis wrote:
> > > > Many places are using their own custom code for handling overflow
> > > > around timestamps or other int64_t values. There are enough of these
> > > > now that having some common saturated math functions seems sound.
> > > >
> > > > This adds implementations that just use the builtin functions for
> > > > recent gcc, clang when available or implements its own version for
> > > > older compilers.
> > >
> > > These look like 64 bit versions of av_sat_add32 and av_sat_sub32, from
> > > common.h, so you should probably add them there and rename them
> > > accordingly.
> > >
> > >
> > Ah! I was looking for those, but missed them. Thanks. Done.
>
> one disadvantage the av_sat* functions have is the lack of inexact
> detection
>
> In addition to av_sat*
> In situations where its better to fail than to clip, something that
> emulates what (+-Inf/)NaN is for float may make sense.
> That would allow to simply check after a computation if any inexactness
> occured
>
> Such a thing could be usefull in situations where a exact value or an
> error is wanted.
>
>
The __builtin functions provide exactly this API, we're just hiding it. I
could add something like:
int did_overflow = av_checked_sat_(add|sub)64(int64_t a, int64_t b,
int64_t* result)

|result| would still satuate and thus av_sat_(add|sub)64 could use it
without checking the return value, but those which want to check and abort
could do so. This is similar to the API shape we expose in Chromium modulo
the fact we enforce an assert.

- dale
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-01 Thread Dale Curtis 
On Fri, May 1, 2020 at 12:50 PM Dale Curtis  wrote:

> On Fri, May 1, 2020 at 12:37 PM Carl Eugen Hoyos 
> wrote:
>
>> >
>> > So ICC on Linux defines __GNUC__ >= 5 yet doesn't support these
>> builtins?
>>
>> No, but yes, this is the effect.
>>
>
> Does this patch work instead on the ICC compiler? GCC 4.2+ have support
> for __has_builtin() which shouldn't be masqueraded by the ICC.
>
>
Whoops, I misread the release notes. It's 6.0
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66970 I guess configure is the
only way unless we limit to GCC6+.

- dale
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-01 Thread Dale Curtis 
On Fri, May 1, 2020 at 12:37 PM Carl Eugen Hoyos  wrote:

> >
> > So ICC on Linux defines __GNUC__ >= 5 yet doesn't support these builtins?
>
> No, but yes, this is the effect.
>

Does this patch work instead on the ICC compiler? GCC 4.2+ have support for
__has_builtin() which shouldn't be masqueraded by the ICC.

- dale
From b52049eea61e602382684534c18fd1e301b13d46 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Fri, 1 May 2020 10:20:43 -0700
Subject: [PATCH] Use gcc/clang builtins for av_sat_(add|sub)_64_c if
 available.

Signed-off-by: Dale Curtis 
---
 libavutil/common.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/libavutil/common.h b/libavutil/common.h
index 11907e5ba7..915e91f8f7 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -299,11 +299,16 @@ static av_always_inline int av_sat_dsub32_c(int a, int b)
  * @return sum with signed saturation
  */
 static av_always_inline int64_t av_sat_add64_c(int64_t a, int64_t b) {
+#if (AV_GCC_VERSION_AT_LEAST(5,0) && __has_builtin(__builtin_add_overflow)) || defined(__clang__)
+  int64_t tmp;
+  return !__builtin_add_overflow(a, b, ) ? tmp : (tmp < 0 ? INT64_MAX : INT64_MIN);
+#else
   if (b >= 0 && a >= INT64_MAX - b)
 return INT64_MAX;
   if (b <= 0 && a <= INT64_MIN - b)
 return INT64_MIN;
   return a + b;
+#endif
 }
 
 /**
@@ -314,11 +319,16 @@ static av_always_inline int64_t av_sat_add64_c(int64_t a, int64_t b) {
  * @return difference with signed saturation
  */
 static av_always_inline int64_t av_sat_sub64_c(int64_t a, int64_t b) {
+#if (AV_GCC_VERSION_AT_LEAST(5,0) && __has_builtin(__builtin_sub_overflow)) || defined(__clang__)
+  int64_t tmp;
+  return !__builtin_sub_overflow(a, b, ) ? tmp : (tmp < 0 ? INT64_MAX : INT64_MIN);
+#else
   if (b <= 0 && a >= INT64_MAX + b) {
 return INT64_MAX;
   if (b >= 0 && a <= INT64_MIN + b) {
 return INT64_MIN;
   return a - b;
+#endif
 }
 
 /**
-- 
2.24.1.windows.2

___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-01 Thread Dale Curtis 
On Fri, May 1, 2020 at 11:22 AM James Almer  wrote:

> On 5/1/2020 3:07 PM, Carl Eugen Hoyos wrote:
> > Am Fr., 1. Mai 2020 um 19:24 Uhr schrieb Dale Curtis <
> dalecur...@chromium.org>:
> >>
> >> Signed-off-by: Dale Curtis 
> >> ---
> >>  libavutil/common.h | 10 ++
> >>  1 file changed, 10 insertions(+)
> >
> > Imo, this is guaranteed to break x86 compilation with some unusual
> > toolchain.
> > I looked carefully at all occurrences of AV_GCC_VERSION_AT_LEAST()
> > and I believe they are in fact different (not for x86 or combined with
> other
> > checks).
>
> Can you elaborate on this? AV_GCC_VERSION_AT_LEAST() is a public macro
> used in public headers to choose one or another path depending on if the
> compiler is a recent enough GCC, or something else.
> What toolchain could this break, and why specifically x86?
> __builtin_*_overflow are arch agnostic GCC functions.
>
>
Yes this is accurate. Actual rebased patch.
https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
https://releases.llvm.org/4.0.0/tools/clang/docs/LanguageExtensions.html#checked-arithmetic-builtins


- dale
From 61af5bd4c9a1f937dd0df5e50127bdd2e41d6965 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Fri, 1 May 2020 10:20:43 -0700
Subject: [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if
 available.

Signed-off-by: Dale Curtis 
---
 libavutil/common.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/libavutil/common.h b/libavutil/common.h
index 11907e5ba7..c4f5eea409 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -299,11 +299,16 @@ static av_always_inline int av_sat_dsub32_c(int a, int b)
  * @return sum with signed saturation
  */
 static av_always_inline int64_t av_sat_add64_c(int64_t a, int64_t b) {
+#if AV_GCC_VERSION_AT_LEAST(5,0) || defined(__clang__)
+  int64_t tmp;
+  return !__builtin_add_overflow(a, b, ) ? tmp : (tmp < 0 ? INT64_MAX : INT64_MIN);
+#else
   if (b >= 0 && a >= INT64_MAX - b)
 return INT64_MAX;
   if (b <= 0 && a <= INT64_MIN - b)
 return INT64_MIN;
   return a + b;
+#endif
 }
 
 /**
@@ -314,11 +319,16 @@ static av_always_inline int64_t av_sat_add64_c(int64_t a, int64_t b) {
  * @return difference with signed saturation
  */
 static av_always_inline int64_t av_sat_sub64_c(int64_t a, int64_t b) {
+#if AV_GCC_VERSION_AT_LEAST(5,0) || defined(__clang__)
+  int64_t tmp;
+  return !__builtin_sub_overflow(a, b, ) ? tmp : (tmp < 0 ? INT64_MAX : INT64_MIN);
+#else
   if (b <= 0 && a >= INT64_MAX + b) {
 return INT64_MAX;
   if (b >= 0 && a <= INT64_MIN + b) {
 return INT64_MIN;
   return a - b;
+#endif
 }
 
 /**
-- 
2.24.1.windows.2

___
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".

Re: [FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-05-01 Thread Dale Curtis 
On Fri, May 1, 2020 at 10:57 AM Carl Eugen Hoyos  wrote:

> Could you confirm that you attached the wrong patch?
>

No, I sent the patches without completing the rebase. Sorry.

- dale


sat_math_v4.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-01 Thread Dale Curtis 
Rebased.

On Fri, May 1, 2020 at 10:24 AM Dale Curtis  wrote:

> Signed-off-by: Dale Curtis 
> ---
>  libavutil/common.h | 10 ++
>  1 file changed, 10 insertions(+)
>


sat_math_builtin_v1.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-05-01 Thread Dale Curtis 
On Fri, May 1, 2020 at 10:32 AM James Almer  wrote:

> On 5/1/2020 2:23 PM, Dale Curtis wrote:
> > On Fri, May 1, 2020 at 6:12 AM James Almer  wrote:
> >
> >> On 5/1/2020 6:36 AM, Carl Eugen Hoyos wrote:
> >>>
> >>> The macro exists to avoid separate patches?
> >>
> >> No, it exists to not require configure checks just to enable a path for
> >> gcc/clang and another for other compilers.
> >>
> >
> > Since consensus seems to have landed on splitting the patches, I've done
> > so. This thread now contains just the default implementation.
>
> That wasn't the consensus. Neither Nicholas or I thought it was
> required, but i don't have strong feelings about it.
>

Ah sorry for misunderstanding, your response to Carl above only seemed to
refute the necessity of a configure patch. I also don't care either way.


> > +static int64_t av_sat_add64_c(int64_t a, int64_t b) {
>
> Missing av_always_inline attribute
>

Done.


>
> > +static int64_t av_sat_sub64_c(int64_t a, int64_t b) {
>
> Ditto
>

Done.

- dale


sat_math_v3.patch
Description: Binary data
___
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".

[FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

2020-05-01 Thread Dale Curtis 
Signed-off-by: Dale Curtis 
---
 libavutil/common.h | 10 ++
 1 file changed, 10 insertions(+)


sat_math_builtin_v0.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-05-01 Thread Dale Curtis 
On Fri, May 1, 2020 at 6:12 AM James Almer  wrote:

> On 5/1/2020 6:36 AM, Carl Eugen Hoyos wrote:
> >
> > The macro exists to avoid separate patches?
>
> No, it exists to not require configure checks just to enable a path for
> gcc/clang and another for other compilers.
>

Since consensus seems to have landed on splitting the patches, I've done
so. This thread now contains just the default implementation.

- dale


sat_math_v2.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-04-30 Thread Dale Curtis 
On Thu, Apr 30, 2020 at 5:21 PM James Almer  wrote:

> On 4/30/2020 7:19 PM, Dale Curtis wrote:
> > Many places are using their own custom code for handling overflow
> > around timestamps or other int64_t values. There are enough of these
> > now that having some common saturated math functions seems sound.
> >
> > This adds implementations that just use the builtin functions for
> > recent gcc, clang when available or implements its own version for
> > older compilers.
>
> These look like 64 bit versions of av_sat_add32 and av_sat_sub32, from
> common.h, so you should probably add them there and rename them
> accordingly.
>
>
Ah! I was looking for those, but missed them. Thanks. Done.


sat_math_v1.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH] [libavformat] Avoid integer overflow on start_time with skip_samples.

2020-04-30 Thread Dale Curtis 
I've sent a follow up patch set implementing saturating operations if
that's something folks are interested in.

- dale

On Thu, Apr 30, 2020 at 2:18 PM Dale Curtis  wrote:

> That said, instead of aborting the operation, perhaps it'd make more sense
> for library functions to be av_saturated_add(), av_saturated_sub() which
> saturate to INT64_MIN/MAX.
>
> - dale
>
> On Thu, Apr 30, 2020 at 1:26 PM Dale Curtis 
> wrote:
>
>> Aside: This overflow check is used in quite a few places now. I wonder if
>> it's worth having a function like the following:
>>
>> int64_t av_no_overflow_add(int64_t a, int64_t b) {
>>   return (a > 0 ? b <= INT64_MAX - a : b >= INT64_MIN - a) ? a + b : a;
>> }
>>
>> Better name suggestions welcome... av_maybe_add_ts?
>>
>> On Thu, Apr 30, 2020 at 1:17 PM Dale Curtis 
>> wrote:
>>
>>> This applies the same workaround used elsewhere in the file for handling
>>> overflow of addition.
>>>
>>> Signed-off-by: Dale Curtis 
>>> ---
>>>  libavformat/utils.c | 15 +++
>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>
___
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".

[FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

2020-04-30 Thread Dale Curtis 
Many places are using their own custom code for handling overflow
around timestamps or other int64_t values. There are enough of these
now that having some common saturated math functions seems sound.

This adds implementations that just use the builtin functions for
recent gcc, clang when available or implements its own version for
older compilers.

Signed-off-by: Dale Curtis 
---
 libavutil/mathematics.c | 26 ++
 libavutil/mathematics.h | 19 +++
 2 files changed, 45 insertions(+)


sat_math_v0.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH] [libavformat] Avoid integer overflow on start_time with skip_samples.

2020-04-30 Thread Dale Curtis 
That said, instead of aborting the operation, perhaps it'd make more sense
for library functions to be av_saturated_add(), av_saturated_sub() which
saturate to INT64_MIN/MAX.

- dale

On Thu, Apr 30, 2020 at 1:26 PM Dale Curtis  wrote:

> Aside: This overflow check is used in quite a few places now. I wonder if
> it's worth having a function like the following:
>
> int64_t av_no_overflow_add(int64_t a, int64_t b) {
>   return (a > 0 ? b <= INT64_MAX - a : b >= INT64_MIN - a) ? a + b : a;
> }
>
> Better name suggestions welcome... av_maybe_add_ts?
>
> On Thu, Apr 30, 2020 at 1:17 PM Dale Curtis 
> wrote:
>
>> This applies the same workaround used elsewhere in the file for handling
>> overflow of addition.
>>
>> Signed-off-by: Dale Curtis 
>> ---
>>  libavformat/utils.c | 15 +++
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>
>
___
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".

Re: [FFmpeg-devel] [PATCH] Don't adjust start time for MP3 files; packets are not adjusted.

2020-04-30 Thread Dale Curtis 
Ping for this patch. Thanks

- dale

On Thu, Apr 23, 2020 at 4:33 PM Dale Curtis  wrote:

> This is a patch Chromium has carried for a while, we forgot to send it
> upstream. 7546ac2fee4 made it so that the start_time for mp3 files is
> adjusted for skip_samples. However, this appears incorrect because
> subsequent packet timestamps are not adjusted and skip_samples are
> applied by deleting data from a packet without changing the timestamp.
>
> E.g., we are told the start_time is ~25ms and we get a packet with a
> timestamp of 0 that has had the skip_samples discarded from it. As such
> rendering engines may incorrectly discard everything prior to the
> 25ms thinking that is where playback should officially start. Since the
> samples were deleted without adjusting timestamps though, the true
> start_time is still 0.
>
> Other formats like MP4 with edit lists will adjust both the start
> time and the timestamps of subsequent packets to avoid this issue.
>
> Signed-off-by: Dale Curtis 
>
___
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".

Re: [FFmpeg-devel] [PATCH] [libavformat] Avoid integer overflow on start_time with skip_samples.

2020-04-30 Thread Dale Curtis 
Aside: This overflow check is used in quite a few places now. I wonder if
it's worth having a function like the following:

int64_t av_no_overflow_add(int64_t a, int64_t b) {
  return (a > 0 ? b <= INT64_MAX - a : b >= INT64_MIN - a) ? a + b : a;
}

Better name suggestions welcome... av_maybe_add_ts?

On Thu, Apr 30, 2020 at 1:17 PM Dale Curtis  wrote:

> This applies the same workaround used elsewhere in the file for handling
> overflow of addition.
>
> Signed-off-by: Dale Curtis 
> ---
>  libavformat/utils.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
___
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".

[FFmpeg-devel] [PATCH] [libavformat] Avoid integer overflow on start_time with skip_samples.

2020-04-30 Thread Dale Curtis 
This applies the same workaround used elsewhere in the file for handling
overflow of addition.

Signed-off-by: Dale Curtis 
---
 libavformat/utils.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)


no_start_time_overflow.patch
Description: Binary data
___
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".

[FFmpeg-devel] [PATCH] Don't adjust start time for MP3 files; packets are not adjusted.

2020-04-23 Thread Dale Curtis 
This is a patch Chromium has carried for a while, we forgot to send it
upstream. 7546ac2fee4 made it so that the start_time for mp3 files is
adjusted for skip_samples. However, this appears incorrect because
subsequent packet timestamps are not adjusted and skip_samples are
applied by deleting data from a packet without changing the timestamp.

E.g., we are told the start_time is ~25ms and we get a packet with a
timestamp of 0 that has had the skip_samples discarded from it. As such
rendering engines may incorrectly discard everything prior to the
25ms thinking that is where playback should officially start. Since the
samples were deleted without adjusting timestamps though, the true
start_time is still 0.

Other formats like MP4 with edit lists will adjust both the start
time and the timestamps of subsequent packets to avoid this issue.

Signed-off-by: Dale Curtis 


no_start_time_adjust_v0.patch
Description: Binary data
___
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".

Re: [FFmpeg-devel] [PATCH] Don't trigger errors for multiple id3 tags.

2020-02-21 Thread Dale Curtis 
On Fri, Feb 21, 2020 at 12:54 PM Dale Curtis 
wrote:

> On Fri, Feb 21, 2020 at 11:26 AM Andreas Rheinhardt <
> andreas.rheinha...@gmail.com> wrote:
>
>> I doubt that this patch still applies as-is because of
>> e2307f4ff197646a7feee0edbcdd2d3262932676.
>>
>>
> Ah, good point. Rebased and attached.
>

Whoops, attached the wrong file.

- dale
From f9f2b953a1e71e439a88581894715568987cba5c Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Fri, 21 Feb 2020 12:53:30 -0800
Subject: [PATCH] Don't trigger errors for multiple id3 tags.

Such errors may make sense for specific formats, but general parsing
logic shouldn't be treating these as errors regardless of the error
recognition mode.

Fixes loading of the following wave when using -err_detect explode:
https://cs.chromium.org/chromium/src/third_party/blink/web_tests/external/wpt/webaudio/resources/4ch-440.wav

Signed-off-by: Dale Curtis 
---
 libavformat/utils.c | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 123d67800b..cb15f6a4b3 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -635,15 +635,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
 s->metadata = s->internal->id3v2_meta;
 s->internal->id3v2_meta = NULL;
 } else if (s->internal->id3v2_meta) {
-int level = AV_LOG_WARNING;
-if (s->error_recognition & AV_EF_COMPLIANT)
-level = AV_LOG_ERROR;
-av_log(s, level, "Discarding ID3 tags because more suitable tags were found.\n");
+av_log(s, AV_LOG_WARNING, "Discarding ID3 tags because more suitable tags were found.\n");
 av_dict_free(>internal->id3v2_meta);
-if (s->error_recognition & AV_EF_EXPLODE) {
-ret = AVERROR_INVALIDDATA;
-goto close;
-}
 }
 
 if (id3v2_extra_meta) {
-- 
2.25.0.265.gbab2e86ba0-goog

___
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".

Re: [FFmpeg-devel] [PATCH] Don't trigger errors for multiple id3 tags.

2020-02-21 Thread Dale Curtis 
On Fri, Feb 21, 2020 at 11:26 AM Andreas Rheinhardt <
andreas.rheinha...@gmail.com> wrote:

> I doubt that this patch still applies as-is because of
> e2307f4ff197646a7feee0edbcdd2d3262932676.
>
>
Ah, good point. Rebased and attached.

- dale
From 57f732774528eecb837467919ec9a284e95470dc Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Fri, 21 Feb 2020 12:53:30 -0800
Subject: [PATCH] Don't trigger errors for multiple id3 tags.

Such errors may make sense for specific formats, but general parsing
logic shouldn't be treating these as errors regardless of the error
recognition mode.

Fixes loading of the following wave when using -err_detect explode:
https://cs.chromium.org/chromium/src/third_party/blink/web_tests/external/wpt/webaudio/resources/4ch-440.wav

Signed-off-by: Dale Curtis 
---
 libavformat/utils.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 123d67800b..72cbfa1690 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -635,15 +635,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
 s->metadata = s->internal->id3v2_meta;
 s->internal->id3v2_meta = NULL;
 } else if (s->internal->id3v2_meta) {
-int level = AV_LOG_WARNING;
-if (s->error_recognition & AV_EF_COMPLIANT)
-level = AV_LOG_ERROR;
 av_log(s, level, "Discarding ID3 tags because more suitable tags were found.\n");
 av_dict_free(>internal->id3v2_meta);
-if (s->error_recognition & AV_EF_EXPLODE) {
-ret = AVERROR_INVALIDDATA;
-goto close;
-}
 }
 
 if (id3v2_extra_meta) {
-- 
2.25.0.265.gbab2e86ba0-goog

___
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".

Re: [FFmpeg-devel] [PATCH] Don't trigger errors for multiple id3 tags.

2020-02-21 Thread Dale Curtis 
+John Rummell  just noticed this patch never landed
upstream. Can this be landed?

- dale

On Thu, Feb 28, 2019 at 1:58 PM Dale Curtis  wrote:

> Such errors may make sense for specific formats, but general parsing
> logic shouldn't be treating these as errors regardless of the error
> recognition mode.
>
> Fixes loading of the following wave when using -err_detect explode:
>
> https://cs.chromium.org/chromium/src/third_party/blink/web_tests/external/wpt/webaudio/resources/4ch-440.wav
>
> Signed-off-by: Dale Curtis 
>
>
___
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".

Re: [FFmpeg-devel] Fix undefined behavior in ff_configure_buffers_for_index()

2020-02-10 Thread Dale Curtis 
On Thu, Feb 6, 2020 at 3:38 PM Michael Niedermayer  wrote:

> On Thu, Jan 30, 2020 at 11:23:07AM -0800, Dale Curtis wrote:
> > On Wed, Jan 29, 2020 at 10:23 PM Michael Niedermayer
> 
> > wrote:
> >
> > > so i think it works but maybe ive missed something, for which values
> > > of e2_pts do you see a problem with e1_pts = INT64_MIN?
> > >
> >
> > For e1_pts = INT64_MIN and e2_pts >= 0 you end up with a negative int64_t
> > result for e2_pts - (uint64_t)e1_pts, so it's always < time_tolerance. If
> > that's what you intended, then sgtm.
>
> thats what the code would do if the elemnts where large enough to not
> overflow
> so that seems to match whats intended.
>
> Do you see some issue here ?
>

Whoops, sorry, I just realized my previous comment was rejected from the
list since I used the wrong e-mail address. My previous statement was
incorrect. The code you proposed correctly promotes a uint64_t and not an
int64_t when used in a conditional. Here's what I wrote that got lost:
"Actually, this was a test construction error on my part. In a conditional
this does evaluate to a uint64_t vs int64_t, so the comparison is valid.
I've attached your recommended patch. Thanks for your patience."

- dale
From c50d0a347fc779c71c07757d9cad8a7d56eb857b Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Tue, 28 Jan 2020 16:49:14 -0800
Subject: [PATCH] Fix undefined behavior in ff_configure_buffers_for_index()

When e2_pts == INT64_MIN and e1_pts >= 0 the calculation of
e2_pts - e1_pts will overflow an int64_t.

Signed-off-by: Dale Curtis 
---
 libavformat/utils.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index e22ca7cab8..81ea239a66 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -2108,6 +2108,8 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance)
 //We could use URLProtocol flags here but as many user applications do not use URLProtocols this would be unreliable
 const char *proto = avio_find_protocol_name(s->url);
 
+av_assert0(time_tolerance >= 0);
+
 if (!proto) {
 av_log(s, AV_LOG_INFO,
"Protocol name not provided, cannot determine if input is local or "
@@ -2135,7 +2137,7 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance)
 for (; i2 < st2->nb_index_entries; i2++) {
 AVIndexEntry *e2 = >index_entries[i2];
 int64_t e2_pts = av_rescale_q(e2->timestamp, st2->time_base, AV_TIME_BASE_Q);
-if (e2_pts - e1_pts < time_tolerance)
+if (e2_pts < e1_pts || e2_pts - (uint64_t)e1_pts < time_tolerance)
 continue;
 pos_delta = FFMAX(pos_delta, e1->pos - e2->pos);
 break;
-- 
2.25.0.341.g760bfbb309-goog

___
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".

Re: [FFmpeg-devel] Fix undefined behavior in ff_configure_buffers_for_index()

2020-02-06 Thread Dale Curtis 
Bump to apply?

- dale

On Thu, Jan 30, 2020 at 3:21 PM Dale Curtis  wrote:

> On Thu, Jan 30, 2020 at 11:23 AM Dale Curtis 
> wrote:
>
>> On Wed, Jan 29, 2020 at 10:23 PM Michael Niedermayer
>>  wrote:
>>
>>> so i think it works but maybe ive missed something, for which values
>>> of e2_pts do you see a problem with e1_pts = INT64_MIN?
>>>
>>
>> For e1_pts = INT64_MIN and e2_pts >= 0 you end up with a negative int64_t
>> result for e2_pts - (uint64_t)e1_pts, so it's always < time_tolerance. If
>> that's what you intended, then sgtm.
>>
>
> Actually, this was a test construction error on my part. In a conditional
> this does evaluate to a uint64_t vs int64_t, so the comparison is valid.
> I've attached your recommended patch. Thanks for your patience.
>
> - dale
>
>
>
___
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".

Re: [FFmpeg-devel] Fix undefined behavior in ff_configure_buffers_for_index()

2020-01-30 Thread Dale Curtis 
On Wed, Jan 29, 2020 at 10:23 PM Michael Niedermayer 
wrote:

> so i think it works but maybe ive missed something, for which values
> of e2_pts do you see a problem with e1_pts = INT64_MIN?
>

For e1_pts = INT64_MIN and e2_pts >= 0 you end up with a negative int64_t
result for e2_pts - (uint64_t)e1_pts, so it's always < time_tolerance. If
that's what you intended, then sgtm.

- dale
___
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".

Re: [FFmpeg-devel] Fix undefined behavior in ff_configure_buffers_for_index()

2020-01-29 Thread Dale Curtis 
On Wed, Jan 29, 2020 at 4:55 AM Michael Niedermayer 
wrote:

> simpler solution, and also behaves arithmetically more correct when the
> overflow happens in the othert direction:
>
> av_assert0(time_tolerance >= 0);
>
> if (e2_pts < e1_pts || e2_pts - (uint64_t)e1_pts < time_tolerance)
>

Does that work? e1_pts is INT64_MIN in this case. So the (uint64_t)e1_pts >
e2_pts.
___
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".

[FFmpeg-devel] Fix undefined behavior in ff_configure_buffers_for_index()

2020-01-28 Thread Dale Curtis 
When e2_pts == INT64_MIN and e1_pts >= 0 the calculation of
e2_pts - e1_pts will overflow an int64_t. So instead check
for overflow and default to |time_tolerance| if the value
is too large for an int64_t.

Signed-off-by: Dale Curtis 
From 412751f4747faf34e3dba088dc55290783eb6bd5 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Tue, 28 Jan 2020 16:49:14 -0800
Subject: [PATCH] Fix undefined behavior in ff_configure_buffers_for_index()

When e2_pts == INT64_MIN and e1_pts >= 0 the calculation of
e2_pts - e1_pts will overflow an int64_t. So instead check
for overflow and default to |time_tolerance| if the value
is too large for an int64_t.

Signed-off-by: Dale Curtis 
---
 libavformat/utils.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index e22ca7cab8..d6197358c9 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -2135,7 +2135,13 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance)
 for (; i2 < st2->nb_index_entries; i2++) {
 AVIndexEntry *e2 = >index_entries[i2];
 int64_t e2_pts = av_rescale_q(e2->timestamp, st2->time_base, AV_TIME_BASE_Q);
-if (e2_pts - e1_pts < time_tolerance)
+int64_t delta = e1_pts < 1 ? INT64_MAX + e1_pts >= e2_pts
+ ? e2_pts - e1_pts
+ : time_tolerance
+   : INT64_MIN + e1_pts <= e2_pts
+ ? e2_pts - e1_pts
+ : time_tolerance;
+if (delta < time_tolerance)
 continue;
 pos_delta = FFMAX(pos_delta, e1->pos - e2->pos);
 break;
-- 
2.25.0.341.g760bfbb309-goog

___
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".

Re: [FFmpeg-devel] [PATCH] lavf/mov: initial support for reading HEIF images

2019-11-12 Thread Dale Curtis 
On Tue, Nov 12, 2019 at 1:27 AM Carl Eugen Hoyos  wrote:

> > Breaks AVIFS files
> > that were previously demuxing as mp4 w/ video track,
>
> Does it only break them once above change was made or also in the existing
> patch?
> (Sorry, I am still travelling in Japan without computer.)
>

Only after I've added AV1 support. However, I suspect HEVC tracks would be
busted in a similar manner.


>
> > i.e.,
> >
> https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Netflix/avifs
> .
> > As such might end up causing some client issues for MP4s that had
> embedded
> > pict elements, but are not HEIF/AVIF - since they'll suddenly start
> > surfacing new tracks.
>
> Can such files easily be detected?
>

Possibly, HEIF/AVIF should declare their brands up front, while a standard
MP4 w/ an embedded PICT or something, probably won't have that. Though I
don't have any samples that would trigger this.
___
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".

Re: [FFmpeg-devel] [PATCH] lavf/mov: initial support for reading HEIF images

2019-11-11 Thread Dale Curtis 
On Fri, Nov 1, 2019 at 4:46 PM Carl Eugen Hoyos  wrote:

> Testing would be helpful.
>
>
Works for AVIF after adding the line I proposed as well as setting
codecpar->width/height for the stream based on ispe. Breaks AVIFS files
that were previously demuxing as mp4 w/ video track, i.e.,
https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/Netflix/avifs.
As such might end up causing some client issues for MP4s that had embedded
pict elements, but are not HEIF/AVIF - since they'll suddenly start
surfacing new tracks.

- dale
___
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".

Re: [FFmpeg-devel] [PATCH] lavf/mov: initial support for reading HEIF images

2019-11-01 Thread Dale Curtis 
On Fri, Nov 1, 2019 at 4:14 AM Swaraj Hota  wrote:

> Sure. Thanks for the samples!
> I have not currently tested the patch with avif format, was only focused on
> heif. But as the only difference is the decoder (?), this support could be
> easily added. I'll try to add it but as I have been working on this for a
> long time now, someone else can surely add it if I couldn't, after my patch
> is merged that is.
>

Should just be adding 'a,'v','0','1' and doing the same thing you do for
hvc1 here:
https://github.com/Swaraj1998/FFmpeg/commit/c20779fea73ed8fa49ce04b99178328e231c9952#diff-44e5be1f9a8e0df35dd607e74773ce6eR6812

Either way sounds good though!

- dale
___
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".

Re: [FFmpeg-devel] [PATCH] lavf/mov: initial support for reading HEIF images

2019-10-31 Thread Dale Curtis 
On Thu, Oct 31, 2019 at 1:32 AM Swaraj Hota  wrote:

> Yes I will send the patch soon for review. Still a few things left to do.
>
> Swaraj
>

Great! Let me know if there's anything I can help with. If you need some
AVIF samples they can be found here:
https://github.com/AOMediaCodec/av1-avif/tree/master/testFiles/

- dale
___
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".

Re: [FFmpeg-devel] [PATCH] lavf/mov: initial support for reading HEIF images

2019-10-30 Thread Dale Curtis 
On Wed, Oct 30, 2019 at 1:17 PM Carl Eugen Hoyos  wrote:

> https://github.com/Swaraj1998/FFmpeg
>
> (Roger's patch sadly did not work with real-world files)


Thanks Carl! That patch looks good. I'll test it out. Is Swaraj still
planning to send that for review later?

- dale
___
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".

Re: [FFmpeg-devel] [PATCH] lavf/mov: initial support for reading HEIF images

2019-10-30 Thread Dale Curtis 
Was there a reason this never landed? I sympathize with the complexity of
the format, but since AVIF has adopted it as well, it'd be nice to have
ffmpeg support.

- dale

On Sat, Aug 19, 2017 at 2:40 AM Carl Eugen Hoyos  wrote:

> 2017-08-19 9:24 GMT+02:00 Rodger Combs :
>
> >  AVInputFormat ff_mov_demuxer = {
> > -.name   = "mov,mp4,m4a,3gp,3g2,mj2",
> > +.name   = "mov,mp4,m4a,3gp,3g2,mj2,heif,heic",
>
> Since it isn't necessary for the new feature:
> Please don't change this line.
>
> Carl Eugen
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
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".

[FFmpeg-devel] [PATCH] Don't trigger errors for multiple id3 tags.

2019-02-28 Thread Dale Curtis 
Such errors may make sense for specific formats, but general parsing
logic shouldn't be treating these as errors regardless of the error
recognition mode.

Fixes loading of the following wave when using -err_detect explode:
https://cs.chromium.org/chromium/src/third_party/blink/web_tests/external/wpt/webaudio/resources/4ch-440.wav

Signed-off-by: Dale Curtis 
From 14c2244631e7d02d6f66a6d5a678125002b89675 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Thu, 28 Feb 2019 13:51:30 -0800
Subject: [PATCH] Don't trigger errors for multiple id3 tags.

Such errors may make sense for specific formats, but general parsing
logic shouldn't be treating these as errors regardless of the error
recognition mode.

Fixes loading of the following wave when using -err_detect explode:
https://cs.chromium.org/chromium/src/third_party/blink/web_tests/external/wpt/webaudio/resources/4ch-440.wav

Signed-off-by: Dale Curtis 
---
 libavformat/utils.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index d113a16c80..b940246512 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -635,13 +635,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
 s->metadata = s->internal->id3v2_meta;
 s->internal->id3v2_meta = NULL;
 } else if (s->internal->id3v2_meta) {
-int level = AV_LOG_WARNING;
-if (s->error_recognition & AV_EF_COMPLIANT)
-level = AV_LOG_ERROR;
-av_log(s, level, "Discarding ID3 tags because more suitable tags were found.\n");
+av_log(s, AV_LOG_WARNING, "Discarding ID3 tags because more suitable tags were found.\n");
 av_dict_free(>internal->id3v2_meta);
-if (s->error_recognition & AV_EF_EXPLODE)
-return AVERROR_INVALIDDATA;
 }
 
 if (id3v2_extra_meta) {
-- 
2.21.0.352.gf09ad66450-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskadec: Check parents remaining length

2019-02-22 Thread Dale Curtis 
Sent http://ffmpeg.org/pipermail/ffmpeg-devel/2019-February/240418.html -
which passes fate and fixes the issue with our test clip.

- dale

On Fri, Feb 22, 2019 at 12:31 PM Dale Curtis 
wrote:

> +steve who submitted the original patch.
>
> - dale
>
>
> On Thu, Feb 21, 2019 at 2:30 PM Dale Curtis 
> wrote:
>
>> One of our test clips is behaving differently after this patch:
>>
>> https://cs.chromium.org/chromium/src/media/test/data/bear-320x240-live.webm
>>
>> The printed log message is:
>> [matroska,webm @ 0x1516c84f4e00] Invalid length 0xff >
>> 0x12f in parent
>>
>> Looking through the code I'm unsure which of the mixed usage
>> "(uint64_t)-1" and "2**56-1" as marker values is correct. Changing the code
>> to:
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 9b706ab4e0..3015a0b230 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -1205,7 +1205,7 @@ static int ebml_parse_elem(MatroskaDemuxContext
>> *matroska,
>>  MatroskaLevel *level =
>> >levels[matroska->num_levels - 1];
>>  AVIOContext *pb = matroska->ctx->pb;
>>  int64_t pos = avio_tell(pb);
>> -if (level->length != (uint64_t) -1 &&
>> +if (level->length != 0xffULL &&
>>  (pos + length) > (level->start + level->length)) {
>>  av_log(matroska->ctx, AV_LOG_ERROR,
>> "Invalid length 0x%"PRIx64" > 0x%"PRIx64" in
>> parent\n",
>>
>> Resolves our issues. Should all other length tests be done the same way?
>>
>> - dale
>>
>> On Sun, Feb 17, 2019 at 12:45 AM Michael Niedermayer 
>> wrote:
>>
>>> On Wed, Feb 13, 2019 at 01:41:31PM +0100, Michael Niedermayer wrote:
>>> > Reported-by: Steve Lhomme
>>> > This was found through the Hacker One program on VLC but is not a
>>> security issue in libavformat
>>> > Signed-off-by: Michael Niedermayer 
>>> > ---
>>> >  libavformat/matroskadec.c | 21 +
>>> >  1 file changed, 21 insertions(+)
>>>
>>> will apply an equivalent variant from steve
>>>
>>> [...]
>>> --
>>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>>
>>> Asymptotically faster algorithms should always be preferred if you have
>>> asymptotical amounts of data
>>> ___
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] Fix handling of unknown length case for matroska files.

2019-02-22 Thread Dale Curtis 
Unknown length has a special encoding which is not uint64_t(-1).

Signed-off-by: Dale Curtis 
---
 libavformat/matroskadec.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)
From 2bf28a1edb54297f44021771b4c3d847c1f923f4 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Fri, 22 Feb 2019 15:39:25 -0800
Subject: [PATCH] Fix handling of unknown length case for matroska files.

Unknown length has a special encoding which is not uint64_t(-1).

Signed-off-by: Dale Curtis 
---
 libavformat/matroskadec.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 5aa8a105dc..6d86342016 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -68,6 +68,9 @@
 
 #include "qtpalette.h"
 
+// 2^56 - 1.
+#define EBML_UNKNOWN_LEN 0xffULL
+
 typedef enum {
 EBML_NONE,
 EBML_UINT,
@@ -869,7 +872,7 @@ static int ebml_read_length(MatroskaDemuxContext *matroska, AVIOContext *pb,
 {
 int res = ebml_read_num(matroska, pb, 8, number);
 if (res > 0 && *number + 1 == 1ULL << (7 * res))
-*number = 0xffULL;
+*number = EBML_UNKNOWN_LEN;
 return res;
 }
 
@@ -1049,7 +1052,7 @@ static int ebml_parse_id(MatroskaDemuxContext *matroska, EbmlSyntax *syntax,
 break;
 if (!syntax[i].id && id == MATROSKA_ID_CLUSTER &&
 matroska->num_levels > 0   &&
-matroska->levels[matroska->num_levels - 1].length == 0xff)
+matroska->levels[matroska->num_levels - 1].length == EBML_UNKNOWN_LEN)
 return 0;  // we reached the end of an unknown size cluster
 if (!syntax[i].id && id != EBML_ID_VOID && id != EBML_ID_CRC32) {
 av_log(matroska->ctx, AV_LOG_DEBUG, "Unknown entry 0x%"PRIX32"\n", id);
@@ -1201,7 +1204,7 @@ static int ebml_parse_elem(MatroskaDemuxContext *matroska,
 MatroskaLevel *level = >levels[matroska->num_levels - 1];
 AVIOContext *pb = matroska->ctx->pb;
 int64_t pos = avio_tell(pb);
-if (level->length != (uint64_t) -1 &&
+if (level->length != EBML_UNKNOWN_LEN &&
 (pos + length) > (level->start + level->length)) {
 av_log(matroska->ctx, AV_LOG_ERROR,
"Invalid length 0x%"PRIx64" > 0x%"PRIx64" in parent\n",
@@ -1610,7 +1613,7 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska,
 ret = AVERROR_INVALIDDATA;
 } else {
 level.start  = 0;
-level.length = (uint64_t) -1;
+level.length = EBML_UNKNOWN_LEN;
 matroska->levels[matroska->num_levels] = level;
 matroska->num_levels++;
 matroska->current_id   = 0;
@@ -1620,7 +1623,7 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska,
 /* remove dummy level */
 while (matroska->num_levels) {
 uint64_t length = matroska->levels[--matroska->num_levels].length;
-if (length == (uint64_t) -1)
+if (length == EBML_UNKNOWN_LEN)
 break;
 }
 }
-- 
2.21.0.rc0.258.g878e2cd30e-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskadec: Check parents remaining length

2019-02-22 Thread Dale Curtis 
+steve who submitted the original patch.

- dale


On Thu, Feb 21, 2019 at 2:30 PM Dale Curtis  wrote:

> One of our test clips is behaving differently after this patch:
> https://cs.chromium.org/chromium/src/media/test/data/bear-320x240-live.webm
>
> The printed log message is:
> [matroska,webm @ 0x1516c84f4e00] Invalid length 0xff >
> 0x12f in parent
>
> Looking through the code I'm unsure which of the mixed usage
> "(uint64_t)-1" and "2**56-1" as marker values is correct. Changing the code
> to:
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 9b706ab4e0..3015a0b230 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1205,7 +1205,7 @@ static int ebml_parse_elem(MatroskaDemuxContext
> *matroska,
>  MatroskaLevel *level = >levels[matroska->num_levels
> - 1];
>  AVIOContext *pb = matroska->ctx->pb;
>  int64_t pos = avio_tell(pb);
> -if (level->length != (uint64_t) -1 &&
> +if (level->length != 0xffULL &&
>  (pos + length) > (level->start + level->length)) {
>  av_log(matroska->ctx, AV_LOG_ERROR,
> "Invalid length 0x%"PRIx64" > 0x%"PRIx64" in
> parent\n",
>
> Resolves our issues. Should all other length tests be done the same way?
>
> - dale
>
> On Sun, Feb 17, 2019 at 12:45 AM Michael Niedermayer 
> wrote:
>
>> On Wed, Feb 13, 2019 at 01:41:31PM +0100, Michael Niedermayer wrote:
>> > Reported-by: Steve Lhomme
>> > This was found through the Hacker One program on VLC but is not a
>> security issue in libavformat
>> > Signed-off-by: Michael Niedermayer 
>> > ---
>> >  libavformat/matroskadec.c | 21 +
>> >  1 file changed, 21 insertions(+)
>>
>> will apply an equivalent variant from steve
>>
>> [...]
>> --
>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> Asymptotically faster algorithms should always be preferred if you have
>> asymptotical amounts of data
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskadec: Check parents remaining length

2019-02-21 Thread Dale Curtis 
One of our test clips is behaving differently after this patch:
https://cs.chromium.org/chromium/src/media/test/data/bear-320x240-live.webm

The printed log message is:
[matroska,webm @ 0x1516c84f4e00] Invalid length 0xff >
0x12f in parent

Looking through the code I'm unsure which of the mixed usage "(uint64_t)-1"
and "2**56-1" as marker values is correct. Changing the code to:
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 9b706ab4e0..3015a0b230 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1205,7 +1205,7 @@ static int ebml_parse_elem(MatroskaDemuxContext
*matroska,
 MatroskaLevel *level = >levels[matroska->num_levels
- 1];
 AVIOContext *pb = matroska->ctx->pb;
 int64_t pos = avio_tell(pb);
-if (level->length != (uint64_t) -1 &&
+if (level->length != 0xffULL &&
 (pos + length) > (level->start + level->length)) {
 av_log(matroska->ctx, AV_LOG_ERROR,
"Invalid length 0x%"PRIx64" > 0x%"PRIx64" in
parent\n",

Resolves our issues. Should all other length tests be done the same way?

- dale

On Sun, Feb 17, 2019 at 12:45 AM Michael Niedermayer 
wrote:

> On Wed, Feb 13, 2019 at 01:41:31PM +0100, Michael Niedermayer wrote:
> > Reported-by: Steve Lhomme
> > This was found through the Hacker One program on VLC but is not a
> security issue in libavformat
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  libavformat/matroskadec.c | 21 +
> >  1 file changed, 21 insertions(+)
>
> will apply an equivalent variant from steve
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Asymptotically faster algorithms should always be preferred if you have
> asymptotical amounts of data
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v3] avcodec: libdav1d AV1 decoder wrapper.

2018-10-26 Thread Dale Curtis 
Yes, all your suggestions silence the errors. Thanks!

- dale

On Fri, Oct 26, 2018 at 4:05 PM James Almer  wrote:

> On 10/26/2018 7:31 PM, Dale Curtis wrote:
> > The following warnings show up when compiling with clang:
> > ../../third_party/ffmpeg/libavcodec/libdav1d.c:92:24: error: suggest
> braces
> > around initialization of subobject [-Werror,-Wmissing-braces]
> > Dav1dPicture p = { 0 };
> >^
> >{}
>
> Does "Dav1dPicture p = { .ref = opaque };" silences this one up?
>
> > ../../third_party/ffmpeg/libavcodec/libdav1d.c:119:24: error: suggest
> > braces around initialization of subobject [-Werror,-Wmissing-braces]
> > Dav1dPicture p = { 0 };
> >^
> >{}
>
> For this one I'd have to use { { 0 } } or a memset, but maybe the struct
> could start with an int or something else instead of the data pointer
> arrays.
>
> > ../../third_party/ffmpeg/libavcodec/libdav1d.c:194:45: error: implicit
> > conversion from enumeration type 'enum Dav1dMatrixCoefficients' to
> > different enumeration type 'enum AVColorSpace'
> [-Werror,-Wenum-conversion]
> > frame->colorspace = c->colorspace = p.p.mtrx;
> >   ~ ^~~~
> > ../../third_party/ffmpeg/libavcodec/libdav1d.c:195:55: error: implicit
> > conversion from enumeration type 'enum Dav1dColorPrimaries' to different
> > enumeration type 'enum AVColorPrimaries' [-Werror,-Wenum-conversion]
> > frame->color_primaries = c->color_primaries = p.p.pri;
> > ~ ^~~
> > ../../third_party/ffmpeg/libavcodec/libdav1d.c:196:43: error: implicit
> > conversion from enumeration type 'enum Dav1dTransferCharacteristics' to
> > different enumeration type 'enum AVColorTransferCharacteristic'
> > [-Werror,-Wenum-conversion]
> > frame->color_trc = c->color_trc = p.p.trc;
> > ~ ^~~
>
> I can cast the enums to silence these errors, sure.
>
> > 5 errors generated.
>
> 5 very pedantic "errors" :p
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v3] avcodec: libdav1d AV1 decoder wrapper.

2018-10-26 Thread Dale Curtis 
On Fri, Oct 26, 2018 at 4:00 PM James Almer  wrote:

> On 10/26/2018 7:50 PM, Dale Curtis wrote:
> > One more piece of feedback, this is not obeying the
> > AVCodecContext.get_buffer2 API.
>
> It's not using it on purpose, wrapping the buffers dav1d allocated
> itself instead. Hence the lack of AV_CODEC_CAP_DR1 flag.
>

Sorry for being unclear, my comment was a request. This is something
Chromium would require to use this wrapper. If it's not planned, we'd forgo
the wrapper and use dav1d directly. Otherwise we'd need to change our
common ffmpeg based decoder enough for this one case that we'd be better
off writing a pure dav1d based decoder. Thanks for your consideration in
any case.

- dale
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v3] avcodec: libdav1d AV1 decoder wrapper.

2018-10-26 Thread Dale Curtis 
One more piece of feedback, this is not obeying the
AVCodecContext.get_buffer2 API.

- dale

On Fri, Oct 26, 2018 at 3:31 PM Dale Curtis  wrote:

> The following warnings show up when compiling with clang:
> ../../third_party/ffmpeg/libavcodec/libdav1d.c:92:24: error: suggest
> braces around initialization of subobject [-Werror,-Wmissing-braces]
> Dav1dPicture p = { 0 };
>^
>{}
> ../../third_party/ffmpeg/libavcodec/libdav1d.c:119:24: error: suggest
> braces around initialization of subobject [-Werror,-Wmissing-braces]
> Dav1dPicture p = { 0 };
>^
>{}
> ../../third_party/ffmpeg/libavcodec/libdav1d.c:194:45: error: implicit
> conversion from enumeration type 'enum Dav1dMatrixCoefficients' to
> different enumeration type 'enum AVColorSpace' [-Werror,-Wenum-conversion]
> frame->colorspace = c->colorspace = p.p.mtrx;
>   ~ ^~~~
> ../../third_party/ffmpeg/libavcodec/libdav1d.c:195:55: error: implicit
> conversion from enumeration type 'enum Dav1dColorPrimaries' to different
> enumeration type 'enum AVColorPrimaries' [-Werror,-Wenum-conversion]
> frame->color_primaries = c->color_primaries = p.p.pri;
> ~ ^~~
> ../../third_party/ffmpeg/libavcodec/libdav1d.c:196:43: error: implicit
> conversion from enumeration type 'enum Dav1dTransferCharacteristics' to
> different enumeration type 'enum AVColorTransferCharacteristic'
> [-Werror,-Wenum-conversion]
> frame->color_trc = c->color_trc = p.p.trc;
> ~ ^~~
> 5 errors generated.
>
>
>
> On Fri, Oct 26, 2018 at 2:59 PM Hendrik Leppkes 
> wrote:
>
>> On Fri, Oct 26, 2018 at 10:46 PM Thierry Foucu  wrote:
>> >
>> > On Wed, Oct 24, 2018 at 4:02 AM Rostislav Pehlivanov <
>> atomnu...@gmail.com>
>> > wrote:
>> >
>> > > On Sat, 20 Oct 2018 at 00:50, James Almer  wrote:
>> > >
>> > > > Originally written by Ronald S. Bultje, with fixes, optimizations
>> and
>> > > > improvements by James Almer.
>> > > >
>> > > > Signed-off-by: James Almer 
>> > > > ---
>> > > > Updated to work with libdav1d git head.
>> > > >
>> > > >  configure  |   4 +
>> > > >  libavcodec/Makefile|   1 +
>> > > >  libavcodec/allcodecs.c |   1 +
>> > > >  libavcodec/libdav1d.c  | 271
>> +
>> > > >  4 files changed, 277 insertions(+)
>> > > >  create mode 100644 libavcodec/libdav1d.c
>> > > >
>> > >
>> > > There hasn't even been a 0.1 release yet, and there won't be one that
>> soon.
>> > > As far as I know the promise is for that to be out by the 30th next
>> month.
>> > > Also I'm still against this making it into the next release.
>> > >
>> >
>> > Why should we wait for a 0.1 release?
>> > If we want to benchmark it against libaom, this is good to have in
>> ffmpeg.
>> > It does not have to be the default decoder...
>> >
>>
>> If there is no guarantees whatsoever for API/ABI stability yet, adding
>> a wrapper already seems a bit early.
>>
>> - Hendrik
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v3] avcodec: libdav1d AV1 decoder wrapper.

2018-10-26 Thread Dale Curtis 
The following warnings show up when compiling with clang:
../../third_party/ffmpeg/libavcodec/libdav1d.c:92:24: error: suggest braces
around initialization of subobject [-Werror,-Wmissing-braces]
Dav1dPicture p = { 0 };
   ^
   {}
../../third_party/ffmpeg/libavcodec/libdav1d.c:119:24: error: suggest
braces around initialization of subobject [-Werror,-Wmissing-braces]
Dav1dPicture p = { 0 };
   ^
   {}
../../third_party/ffmpeg/libavcodec/libdav1d.c:194:45: error: implicit
conversion from enumeration type 'enum Dav1dMatrixCoefficients' to
different enumeration type 'enum AVColorSpace' [-Werror,-Wenum-conversion]
frame->colorspace = c->colorspace = p.p.mtrx;
  ~ ^~~~
../../third_party/ffmpeg/libavcodec/libdav1d.c:195:55: error: implicit
conversion from enumeration type 'enum Dav1dColorPrimaries' to different
enumeration type 'enum AVColorPrimaries' [-Werror,-Wenum-conversion]
frame->color_primaries = c->color_primaries = p.p.pri;
~ ^~~
../../third_party/ffmpeg/libavcodec/libdav1d.c:196:43: error: implicit
conversion from enumeration type 'enum Dav1dTransferCharacteristics' to
different enumeration type 'enum AVColorTransferCharacteristic'
[-Werror,-Wenum-conversion]
frame->color_trc = c->color_trc = p.p.trc;
~ ^~~
5 errors generated.



On Fri, Oct 26, 2018 at 2:59 PM Hendrik Leppkes  wrote:

> On Fri, Oct 26, 2018 at 10:46 PM Thierry Foucu  wrote:
> >
> > On Wed, Oct 24, 2018 at 4:02 AM Rostislav Pehlivanov <
> atomnu...@gmail.com>
> > wrote:
> >
> > > On Sat, 20 Oct 2018 at 00:50, James Almer  wrote:
> > >
> > > > Originally written by Ronald S. Bultje, with fixes, optimizations and
> > > > improvements by James Almer.
> > > >
> > > > Signed-off-by: James Almer 
> > > > ---
> > > > Updated to work with libdav1d git head.
> > > >
> > > >  configure  |   4 +
> > > >  libavcodec/Makefile|   1 +
> > > >  libavcodec/allcodecs.c |   1 +
> > > >  libavcodec/libdav1d.c  | 271
> +
> > > >  4 files changed, 277 insertions(+)
> > > >  create mode 100644 libavcodec/libdav1d.c
> > > >
> > >
> > > There hasn't even been a 0.1 release yet, and there won't be one that
> soon.
> > > As far as I know the promise is for that to be out by the 30th next
> month.
> > > Also I'm still against this making it into the next release.
> > >
> >
> > Why should we wait for a 0.1 release?
> > If we want to benchmark it against libaom, this is good to have in
> ffmpeg.
> > It does not have to be the default decoder...
> >
>
> If there is no guarantees whatsoever for API/ABI stability yet, adding
> a wrapper already seems a bit early.
>
> - Hendrik
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v3] avcodec: libdav1d AV1 decoder wrapper.

2018-10-23 Thread Dale Curtis 
On Fri, Oct 19, 2018 at 4:50 PM James Almer  wrote:

> +s.n_tile_threads = dav1d->tile_threads;
> +s.n_frame_threads = dav1d->frame_threads;
>

Did you consider using the AVCodecContext.threads value along
AVCodecContext.thread_type flags? That seems to be how this is handled
elsewhere.

- dale
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Reduce static table size for VLC tables in h264_cavlc.c

2018-09-17 Thread Dale Curtis 
On Sat, Sep 8, 2018 at 5:49 PM Michael Niedermayer 
wrote:

> dont all modern OS assign physical memory only once something is stored
> in these tables?
>

This seems to be correct. I was misreading the tooling which indicated
these were taking up size. So this patch can be abandoned. Sorry for the
noise!

- dale
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] Reduce static table size for VLC tables in h264_cavlc.c

2018-09-07 Thread Dale Curtis 
These tables represent ~70k so moving the allocation to when
they're actually used reduces memory usage in cases where the
h264 decoder isn't used.
From e1cbe52a1f41a39698136efb4695d8d019117853 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Fri, 31 Aug 2018 16:50:23 -0700
Subject: [PATCH] Reduce static table size for VLC tables in h264_cavlc.c

These tables represent ~70k so moving the allocation to when
they're actually used reduces memory usage in cases where the
h264 decoder isn't used.

Signed-off-by: Dale Curtis 
---
 libavcodec/h264_cavlc.c | 43 +
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/libavcodec/h264_cavlc.c b/libavcodec/h264_cavlc.c
index a7e60676a0..7769202401 100644
--- a/libavcodec/h264_cavlc.c
+++ b/libavcodec/h264_cavlc.c
@@ -236,35 +236,35 @@ static const uint8_t run_bits[7][16]={
 };
 
 static VLC coeff_token_vlc[4];
-static VLC_TYPE coeff_token_vlc_tables[520+332+280+256][2];
+static VLC_TYPE (*coeff_token_vlc_tables)[520+332+280+256][2];
 static const int coeff_token_vlc_tables_size[4]={520,332,280,256};
 
 static VLC chroma_dc_coeff_token_vlc;
-static VLC_TYPE chroma_dc_coeff_token_vlc_table[256][2];
+static VLC_TYPE (*chroma_dc_coeff_token_vlc_table)[256][2];
 static const int chroma_dc_coeff_token_vlc_table_size = 256;
 
 static VLC chroma422_dc_coeff_token_vlc;
-static VLC_TYPE chroma422_dc_coeff_token_vlc_table[8192][2];
+static VLC_TYPE (*chroma422_dc_coeff_token_vlc_table)[8192][2];
 static const int chroma422_dc_coeff_token_vlc_table_size = 8192;
 
 static VLC total_zeros_vlc[15+1];
-static VLC_TYPE total_zeros_vlc_tables[15][512][2];
+static VLC_TYPE (*total_zeros_vlc_tables)[15][512][2];
 static const int total_zeros_vlc_tables_size = 512;
 
 static VLC chroma_dc_total_zeros_vlc[3+1];
-static VLC_TYPE chroma_dc_total_zeros_vlc_tables[3][8][2];
+static VLC_TYPE (*chroma_dc_total_zeros_vlc_tables)[3][8][2];
 static const int chroma_dc_total_zeros_vlc_tables_size = 8;
 
 static VLC chroma422_dc_total_zeros_vlc[7+1];
-static VLC_TYPE chroma422_dc_total_zeros_vlc_tables[7][32][2];
+static VLC_TYPE (*chroma422_dc_total_zeros_vlc_tables)[7][32][2];
 static const int chroma422_dc_total_zeros_vlc_tables_size = 32;
 
 static VLC run_vlc[6+1];
-static VLC_TYPE run_vlc_tables[6][8][2];
+static VLC_TYPE (*run_vlc_tables)[6][8][2];
 static const int run_vlc_tables_size = 8;
 
 static VLC run7_vlc;
-static VLC_TYPE run7_vlc_table[96][2];
+static VLC_TYPE (*run7_vlc_table)[96][2];
 static const int run7_vlc_table_size = 96;
 
 #define LEVEL_TAB_BITS 8
@@ -331,14 +331,23 @@ av_cold void ff_h264_decode_init_vlc(void){
 int offset;
 done = 1;
 
-chroma_dc_coeff_token_vlc.table = chroma_dc_coeff_token_vlc_table;
+coeff_token_vlc_tables = av_mallocz(sizeof(*coeff_token_vlc_tables));
+chroma_dc_coeff_token_vlc_table = av_mallocz(sizeof(*chroma_dc_coeff_token_vlc_table));
+chroma422_dc_coeff_token_vlc_table = av_mallocz(sizeof(*chroma422_dc_coeff_token_vlc_table));
+total_zeros_vlc_tables = av_mallocz(sizeof(*total_zeros_vlc_tables));
+chroma_dc_total_zeros_vlc_tables = av_mallocz(sizeof(*chroma_dc_total_zeros_vlc_tables));
+chroma422_dc_total_zeros_vlc_tables = av_mallocz(sizeof(*chroma422_dc_total_zeros_vlc_tables));
+run_vlc_tables = av_mallocz(sizeof(*run_vlc_tables));
+run7_vlc_table = av_mallocz(sizeof(*run7_vlc_table));
+
+chroma_dc_coeff_token_vlc.table = *chroma_dc_coeff_token_vlc_table;
 chroma_dc_coeff_token_vlc.table_allocated = chroma_dc_coeff_token_vlc_table_size;
 init_vlc(_dc_coeff_token_vlc, CHROMA_DC_COEFF_TOKEN_VLC_BITS, 4*5,
  _dc_coeff_token_len [0], 1, 1,
  _dc_coeff_token_bits[0], 1, 1,
  INIT_VLC_USE_NEW_STATIC);
 
-chroma422_dc_coeff_token_vlc.table = chroma422_dc_coeff_token_vlc_table;
+chroma422_dc_coeff_token_vlc.table = *chroma422_dc_coeff_token_vlc_table;
 chroma422_dc_coeff_token_vlc.table_allocated = chroma422_dc_coeff_token_vlc_table_size;
 init_vlc(_dc_coeff_token_vlc, CHROMA422_DC_COEFF_TOKEN_VLC_BITS, 4*9,
  _dc_coeff_token_len [0], 1, 1,
@@ -347,7 +356,7 @@ av_cold void ff_h264_decode_init_vlc(void){
 
 offset = 0;
 for(i=0; i<4; i++){
-coeff_token_vlc[i].table = coeff_token_vlc_tables+offset;
+coeff_token_vlc[i].table = (*coeff_token_vlc_tables)+offset;
 coeff_token_vlc[i].table_allocated = coeff_token_vlc_tables_size[i];
 init_vlc(_token_vlc[i], COEFF_TOKEN_VLC_BITS, 4*17,
  _token_len [i][0], 1, 1,
@@ -360,10 +369,10 @@ av_cold void ff_h264_decode_init_vlc(void){
  * the packed static coeff_token_vlc table sizes
  * were initialized correctly.
  */
-av_assert0(offset == FF_ARRAY_ELEMS(coeff_token_vlc_tables));
+av_assert0(offset == FF_ARRAY_EL

[FFmpeg-devel] Don't calculate duration using AV_NOPTS_VALUE for start_time.

2018-09-07 Thread Dale Curtis 
Found by ClusterFuzz, https://crbug.com/879852
From 68614e9a099ee4ae754da5fa36fbb6a570f4aa73 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Fri, 7 Sep 2018 15:37:09 -0700
Subject: [PATCH] Don't calculate duration using AV_NOPTS_VALUE for start_time.

Found by ClusterFuzz, https://crbug.com/879852
---
 libavformat/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 15c0c4bddc..8e0bd5d697 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -3869,7 +3869,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
 break;
 }
 if (pkt->duration) {
-if (avctx->codec_type == AVMEDIA_TYPE_SUBTITLE && pkt->pts != AV_NOPTS_VALUE && pkt->pts >= st->start_time) {
+if (avctx->codec_type == AVMEDIA_TYPE_SUBTITLE && pkt->pts != AV_NOPTS_VALUE && st->start_time != AV_NOPTS_VALUE && pkt->pts >= st->start_time) {
 st->info->codec_info_duration = FFMIN(pkt->pts - st->start_time, st->info->codec_info_duration + pkt->duration);
 } else
 st->info->codec_info_duration += pkt->duration;
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [mov] Error on too large stsd entry counts.

2018-08-30 Thread Dale Curtis 
Entries are always at least 8 bytes per the parsing code, so if we
see an impossible entry count avoid massive allocations. This is
similar to an existing check in mov_read_stsc().

Since ff_mov_read_stsd_entries() does eof checks, an alternative
approach could be to clamp the entry count to atom.size / 8.

Signed-off-by: Dale Curtis 
From 3e1663d84068ff7615f7e84fa1c1122729a531da Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Thu, 30 Aug 2018 15:18:25 -0700
Subject: [PATCH] Error on too large stsd entry counts.

Entries are always at least 8 bytes per the parsing code, so if we
see an impossible entry count avoid massive allocations. This is
similar to an existing check in mov_read_stsc().

Since ff_mov_read_stsd_entries() does eof checks, an alternative
approach could be to clamp the entry count to atom.size / 8.

Signed-off-by: Dale Curtis 
---
 libavformat/mov.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index d66f4e338c..d922e0f173 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2558,7 +2558,8 @@ static int mov_read_stsd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 avio_rb24(pb); /* flags */
 entries = avio_rb32(pb);
 
-if (entries <= 0) {
+/* Each entry contains a size (4 bytes) and format (4 bytes). */
+if (entries <= 0 || entries > atom.size / 8) {
 av_log(c->fc, AV_LOG_ERROR, "invalid STSD entries %d\n", entries);
 return AVERROR_INVALIDDATA;
 }
-- 
2.19.0.rc0.228.g281dcd1b4d0-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] Correct opus-in-mp4 pre-skip to be uint16_t versus int16_t.

2018-08-21 Thread Dale Curtis 
This field is a uint16_t, see docs:
http://opus-codec.org/docs/opus_in_isobmff.html#4.3.2

Signed-off-by: Dale Curtis 
From 7f1588bc92ef4a70025aa140a8e660a36875c89c Mon Sep 17 00:00:00 2001
From: Dale Curtis 
Date: Tue, 21 Aug 2018 15:42:31 -0700
Subject: [PATCH] Correct opus-in-mp4 pre-skip to be uint16_t versus int16_t.

This field is a uint16_t, see docs:
http://opus-codec.org/docs/opus_in_isobmff.html#4.3.2

Signed-off-by: Dale Curtis 
---
 libavformat/mov.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 1bd7d7e483..02f6c1e0a1 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -6608,7 +6608,7 @@ static int mov_read_dops(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 const int OPUS_SEEK_PREROLL_MS = 80;
 AVStream *st;
 size_t size;
-int16_t pre_skip;
+uint16_t pre_skip;
 
 if (c->fc->nb_streams < 1)
 return 0;
-- 
2.18.0.1017.ga543ac7ca45-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Respect AR and NM overrides for Windows builds.

2018-04-17 Thread Dale Curtis 
Yes, to cross-compile on Linux you need to use llvm-ar and llvm-nm. This is
pretty chromium specific, but you can get the gist of what's necessary from
this Chromium change:

https://chromium-review.googlesource.com/c/chromium/third_party/ffmpeg/+/1013323/4/chromium/scripts/build_ffmpeg.py

- dale

On Tue, Apr 17, 2018 at 6:00 AM Derek Buitenhuis <derek.buitenh...@gmail.com>
wrote:

> On 4/17/2018 12:28 AM, Dale Curtis wrote:
> > Necessary for clang-cl cross compiling builds to work on Linux.
>
> Looks fairly reasonable, I think.
>
> Are you manually overriding these?
>
> - Derek
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

  1   2   >