Re: [FFmpeg-devel] [PATCH] avformat/mxfdec: fix last packet timestamps

[Derek Buitenhuis]

On 11/22/2017 1:10 AM, Marton Balint wrote:
> Fixes the packet timestamps of the last packet, which was unset, or guessed by
> compute_pkt_fields.
> 
> ffprobe -fflags nofillin -show_packets tests/data/lavf/lavf.mxf 
> -select_streams v
> 
> Signed-off-by: Marton Balint 
> ---
>  libavformat/mxfdec.c|  8 
>  tests/ref/seek/lavf-mxf_d10 | 12 ++--
>  2 files changed, 10 insertions(+), 10 deletions(-)

Can you add what the fix actually was to the commit message?

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

Re: [FFmpeg-devel] [PATCH] avfilter: slice processing for geq

[Derek Buitenhuis]

On 11/22/2017 10:24 AM, Marc-Antoine ARNAUD wrote:
> New patch version which fixe the last remark.

[...]

> +char depth_string[8];
> +snprintf(depth_string, sizeof(depth_string), "%d", (1 1);
> +geq->expr_str[A] = av_strdup(depth_string);

Missing return value check for av_strdup.

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

Re: [FFmpeg-devel] [PATCH] avfilter: slice processing for geq

[Derek Buitenhuis]

On 11/22/2017 2:38 PM, Marc-Antoine ARNAUD wrote:
> It made sense, but this commit don't touch this part of the code.
> It can made more sense to add an another path to "prevent bad memory
> allocation in geq filter". What do you think ?

Ah, you are correct. A 2nd patch would be nice, yes, but don't consider
it a blocker for your current patch.

Sorry 'bout that!

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

[FFmpeg-devel] [PATCH 0/3][RFC] Random UDP bits

[Derek Buitenhuis]

Bits that I saw when I was reviewing the OpenSRT code.

Derek Buitenhuis (3):
  Revert "udp: fix compilation when HAVE_PTHREAD_CANCEL isnt defined"
  udp: Check the port number provided by av_url_split as per docs
  udp: Actually fail when we're missing required options, like the
"warning" says.

 libavformat/udp.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

-- 
1.8.3.1

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

[FFmpeg-devel] [PATCH 1/3] Revert "udp: fix compilation when HAVE_PTHREAD_CANCEL isnt defined"

[Derek Buitenhuis]

This was an mplayer-specific hack.

This reverts commit a4f94f24b4f153c30bbcaa700bedfb2b3a581e5e.
---
Near as I can tell, this is a hack added to deal with the fact that the
terrible mplayer build system cannibalized FFmpeg's, and failed at it.

There's no reason this should be in FFmpeg, if that is the case
---
 libavformat/udp.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/libavformat/udp.c b/libavformat/udp.c
index 3835f98..0dde035 100644
--- a/libavformat/udp.c
+++ b/libavformat/udp.c
@@ -64,10 +64,6 @@
 #include 
 #endif
 
-#ifndef HAVE_PTHREAD_CANCEL
-#define HAVE_PTHREAD_CANCEL 0
-#endif
-
 #ifndef IPV6_ADD_MEMBERSHIP
 #define IPV6_ADD_MEMBERSHIP IPV6_JOIN_GROUP
 #define IPV6_DROP_MEMBERSHIP IPV6_LEAVE_GROUP
-- 
1.8.3.1

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

[FFmpeg-devel] [PATCH 3/3] udp: Actually fail when we're missing required options, like the "warning" says.

[Derek Buitenhuis]

Signed-off-by: Derek Buitenhuis 
---
There was no reasoning in the commit that added this, so maybe someone on
the list has some insights.
---
 libavformat/udp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/udp.c b/libavformat/udp.c
index 7bbd282..6319655 100644
--- a/libavformat/udp.c
+++ b/libavformat/udp.c
@@ -963,8 +963,8 @@ static int udp_open(URLContext *h, const char *uri, int 
flags)
 */
 
 if (is_output && s->bitrate && !s->circular_buffer_size) {
-/* Warn user in case of 'circular_buffer_size' is not set */
-av_log(h, AV_LOG_WARNING,"'bitrate' option was set but 
'circular_buffer_size' is not, but required\n");
+av_log(h, AV_LOG_ERROR,"'bitrate' option was set but 
'circular_buffer_size' is not, but required\n");
+goto fail;
 }
 
 if ((!is_output && s->circular_buffer_size) || (is_output && s->bitrate && 
s->circular_buffer_size)) {
-- 
1.8.3.1

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

[FFmpeg-devel] [PATCH 2/3] udp: Check the port number provided by av_url_split as per docs

[Derek Buitenhuis]

Signed-off-by: Derek Buitenhuis 
---
 libavformat/udp.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/libavformat/udp.c b/libavformat/udp.c
index 0dde035..7bbd282 100644
--- a/libavformat/udp.c
+++ b/libavformat/udp.c
@@ -443,6 +443,10 @@ int ff_udp_set_remote_url(URLContext *h, const char *uri)
 const char *p;
 
 av_url_split(NULL, 0, NULL, 0, hostname, sizeof(hostname), &port, NULL, 0, 
uri);
+if (port < 0) {
+av_log(s, AV_LOG_ERROR, "No valid port number found in URL.\n");
+return AVERROR(EINVAL);
+}
 
 /* set the destination address */
 s->dest_addr_len = udp_set_url(h, &s->dest_addr, hostname, port);
@@ -798,6 +802,10 @@ static int udp_open(URLContext *h, const char *uri, int 
flags)
 
 /* fill the dest addr */
 av_url_split(NULL, 0, NULL, 0, hostname, sizeof(hostname), &port, NULL, 0, 
uri);
+if (port < 0) {
+av_log(h, AV_LOG_ERROR, "Missing or invalid port provided in URL.\n");
+return AVERROR(EINVAL);
+}
 
 /* XXX: fix av_url_split */
 if (hostname[0] == '\0' || hostname[0] == '?') {
-- 
1.8.3.1

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

Re: [FFmpeg-devel] [PATCH] avformat/avienc: fix fields-per-frame value for interlaced video streams

[Derek Buitenhuis]

On 11/22/2017 3:41 PM, Tobias Rapp wrote:
> Writes one set of field framing information for progressive streams and
> two sets for interlaced streams. Fixes ticket #6383.
> 
> Unfortunately the OpenDML v1.02 document is not very specific what value
> to use for start_line when frame data is not coming from a capturing
> device, so this is just using 0/1 depending on the field order as a
> best-effort guess.

Looking at the OpenDML spec, I think this indeed is the best we can do without
deeper knowledge of where the source signal came from, or copying it from a
pre-existing file, within the existing avformat scaffolding.

Code itself seems OK.

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

Re: [FFmpeg-devel] [PATCH] libavformat/mov: Replace duplicate stream_nb check by assert

[Derek Buitenhuis]

On 11/22/2017 8:09 PM, Michael Niedermayer wrote:
> not much, no
> its a non static function tough
> i can remove the check completely if thats preferred ?

I guess leave it since it's non-static.

LGTM.

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

Re: [FFmpeg-devel] [PATCH] avformat/avienc: fix fields-per-frame value for interlaced video streams

[Derek Buitenhuis]

On 11/22/2017 10:52 PM, Carl Eugen Hoyos wrote:
> start_line = fields * (i ^ (par->field_order == AV_FIELD_BB ||
> par->field_order == AV_FIELD_BT));
> 
> Which are imo less ugly.

Can't agree.

It's needlessly less readable bit fiddling.

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

Re: [FFmpeg-devel] [PATCH] Refactor Developer Docs, update dev list section

[Derek Buitenhuis]

On 11/22/2017 11:34 PM, Carl Eugen Hoyos wrote:
> Please understand I am against removing the paragraph from the
> documentation because I believe it is a good idea if developers
> are subscribed to -cvslog.

Perhaps it can be reworded a bit to say it's encouraged for the
cited reasons, but not mandatory if you just want to send a patch
here and there?

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

Re: [FFmpeg-devel] [PATCH]ffmpeg: Improve warnings if av_buffersrc_add_frame*() fail

[Derek Buitenhuis]

On 11/23/2017 11:09 AM, Carl Eugen Hoyos wrote:
> Hi!
> 
> Attached patch implements more verbose warnings as suggested by Nicolas.
> 
> Please comment, Carl Eugen

OK.

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

Re: [FFmpeg-devel] [PATCH 1/3] Revert "udp: fix compilation when HAVE_PTHREAD_CANCEL isnt defined"

[Derek Buitenhuis]

On 11/22/2017 4:41 PM, Paul B Mahol wrote:
> LGTM

Pushed.

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

Re: [FFmpeg-devel] [PATCH 2/3] udp: Check the port number provided by av_url_split as per docs

[Derek Buitenhuis]

On 11/23/2017 11:28 PM, Michael Niedermayer wrote:
> this seems to break rtp / rtsp
> 
> ive traced it to
> ff_rtsp_make_setup_request() calling ffurl_open_whitelist() without
> a port

Looking at:


https://github.com/FFmpeg/FFmpeg/blob/1e27837265702b63db65122e97178a0ca4d25e05/libavformat/rtsp.c#L1466-L1476

It seems it's using '?localport=' instead of specifying a port...

Is there a particular reason for this? It seems... odd. I don't know the RTSP
code, really.

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

Re: [FFmpeg-devel] [PATCH] avfilter: add lv2 wrapper filter

[Derek Buitenhuis]

On 11/23/2017 9:16 PM, Paul B Mahol wrote:

> +typedef struct LV2Context {
> +const AVClass *class;
> +char *plugin_uri;
> +char *options;
> +
> +unsigned long nb_inputs;
> +unsigned long nb_inputcontrols;
> +unsigned long nb_outputs;

Why are you using longs instead of stdint?

> +table->uris = av_realloc(table->uris, ++table->n_uris * sizeof(char*));
> +table->uris[table->n_uris - 1] = av_malloc(len + 1);

Every single allocation in this whole file is completely unchecked. That's not 
OK.

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

[FFmpeg-devel] [PATCH 0/2] alloc NULL check fuzzer finds

[Derek Buitenhuis]

This was from a single FATE pass, an only 2 of the 10+ found.

Going to send the tool I use to fuzz it to the list as well.

Derek Buitenhuis (2):
  h264_picture: Actually return error during alloc failure
  vorbisenc: Check the return value of av_frame_clone

 libavcodec/h264_picture.c | 12 +---
 libavcodec/vorbisenc.c|  6 +-
 2 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.15.0

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

[FFmpeg-devel] [PATCH 2/2] vorbisenc: Check the return value of av_frame_clone

[Derek Buitenhuis]

Prevents a segfault when alloc fails.

Signed-off-by: Derek Buitenhuis 
---
 libavcodec/vorbisenc.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavcodec/vorbisenc.c b/libavcodec/vorbisenc.c
index a4ecd8f754..18a679f2dc 100644
--- a/libavcodec/vorbisenc.c
+++ b/libavcodec/vorbisenc.c
@@ -1093,9 +1093,13 @@ static int vorbis_encode_frame(AVCodecContext *avctx, 
AVPacket *avpkt,
 PutBitContext pb;
 
 if (frame) {
+AVFrame *clone;
 if ((ret = ff_af_queue_add(&venc->afq, frame)) < 0)
 return ret;
-ff_bufqueue_add(avctx, &venc->bufqueue, av_frame_clone(frame));
+clone = av_frame_clone(frame);
+if (!clone)
+return AVERROR(ENOMEM);
+ff_bufqueue_add(avctx, &venc->bufqueue, clone);
 } else
 if (!venc->afq.remaining_samples)
 return 0;
-- 
2.15.0

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

[FFmpeg-devel] [PATCH 1/2] h264_picture: Actually return error during alloc failure

[Derek Buitenhuis]

Fixes NULL dereference during alloc failure.

Signed-off-by: Derek Buitenhuis 
---
 libavcodec/h264_picture.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/libavcodec/h264_picture.c b/libavcodec/h264_picture.c
index e7dd84bc47..e833835a77 100644
--- a/libavcodec/h264_picture.c
+++ b/libavcodec/h264_picture.c
@@ -78,24 +78,30 @@ int ff_h264_ref_picture(H264Context *h, H264Picture *dst, 
H264Picture *src)
 
 dst->qscale_table_buf = av_buffer_ref(src->qscale_table_buf);
 dst->mb_type_buf  = av_buffer_ref(src->mb_type_buf);
-if (!dst->qscale_table_buf || !dst->mb_type_buf)
+if (!dst->qscale_table_buf || !dst->mb_type_buf) {
+ret = AVERROR(ENOMEM);
 goto fail;
+}
 dst->qscale_table = src->qscale_table;
 dst->mb_type  = src->mb_type;
 
 for (i = 0; i < 2; i++) {
 dst->motion_val_buf[i] = av_buffer_ref(src->motion_val_buf[i]);
 dst->ref_index_buf[i]  = av_buffer_ref(src->ref_index_buf[i]);
-if (!dst->motion_val_buf[i] || !dst->ref_index_buf[i])
+if (!dst->motion_val_buf[i] || !dst->ref_index_buf[i]) {
+ret = AVERROR(ENOMEM);
 goto fail;
+}
 dst->motion_val[i] = src->motion_val[i];
 dst->ref_index[i]  = src->ref_index[i];
 }
 
 if (src->hwaccel_picture_private) {
 dst->hwaccel_priv_buf = av_buffer_ref(src->hwaccel_priv_buf);
-if (!dst->hwaccel_priv_buf)
+if (!dst->hwaccel_priv_buf) {
+ret = AVERROR(ENOMEM);
 goto fail;
+}
 dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data;
 }
 
-- 
2.15.0

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

[FFmpeg-devel] [PATCH 0/1][TOOL][HACK] Allocation NULL check fuzzer

[Derek Buitenhuis]

I've had this kicking around for like 4 years, maybe it can be of use to some 
people.
I haven't done full scale fuzzing with this because laziness. I just sometimes 
run it
when I'm bored. It's not thread-safe, but it would be trivial to make it so.

It's based off my old LD_PRELOAD hack from here:

https://gist.github.com/dwbuiten/7101755

Optionally takes two env vars, MALLOC_SEED (the seed), and MALLOC_FAILPROB for 
the
probability of failing.

I've been running it directly integrated inside FFmpeg's allocator because it 
makes
it easier to run under gdb to find where it actually crashes, if the stack 
trace of
the failure is not enough info/context.

Currently FFmpeg has a lot of unchecked allocations - just one single FATE run 
with
this found:

daemon404@bbvm:~/dev/f/ffmpeg/tests/data/fate$ grep Seg *.err
adpcm-ima-amv.err:Segmentation fault
adpcm-ima-apc.err:Segmentation fault
caf.err:Segmentation fault
filter-mergeplanes.err:Segmentation fault
filter-paletteuse-bayer.err:Segmentation fault
filter-paletteuse-sierra2_4a.err:Segmentation fault
lavf-wav.err:Segmentation fault
lavf-wav_peak.err:Segmentation fault
lavf-wav_peak_only.err:Segmentation fault
vorbis-encode.err:Segmentation fault
vsynth1-msvideo1.err:Segmentation fault

Plus an infinite EAGAIN loop with av_usleep ffmpeg.c, that I'm still trying to
debug.

It prints a stack trace when it fails so that you know which allocation failed:

daemon404@bbvm:~/dev/f/ffmpeg/tests/data/fate$ cat vorbis-encode.err

[... snip ...]

FAILED. Iteration = 9789, Seed = 1511549218.

my_posix_memalign:77 in libavutil/posixmemalign.c
av_malloc:89 in libavutil/mem.c
av_mallocz:240 in libavutil/mem.c
av_frame_alloc:152 in libavutil/frame.c
av_frame_clone:499 in libavutil/frame.c
vorbis_encode_frame:1098 in libavcodec/vorbisenc.c
avcodec_encode_audio2:198 in libavcodec/encode.c
do_encode:377 in libavcodec/encode.c
avcodec_send_frame:423 in libavcodec/encode.c
do_audio_out:931 in fftools/ffmpeg.c
reap_filters:1507 in fftools/ffmpeg.c
transcode_step:4562 in fftools/ffmpeg.c
transcode:4606 in fftools/ffmpeg.c
main:4812 in fftools/ffmpeg.c
(null):0 in (null)
(null):0 in (null)
(null):0 in (null)
Segmentation fault

The (null) stuff is noise from libbacktrace, and could be skipped with a proper
callback.

And e.g. to reproduce this under gdb:

$ MALLOC_SEED=1511549218 gdb --args /home/daemon404/dev/f/ffmpeg/ffmpeg_g \
  -nostdin -nostats -cpuflags all -hwaccel none -threads 1 -thread_type 
frame+slice \ 
  -i /home/daemon404/fate/audio-reference/luckynight_2ch_44kHz_s16.wav -c:a 
vorbis \
  -strict experimental -f ogg -y 
/home/daemon404/dev/f/ffmpeg/tests/data/fate/vorbis-encode.ogg

(Obtained from 'make fate-vorbis-encode V=1'.)

Currently it's using libbacktrace from GCC, which can be build entirely 
separately from
GDB (cd libbacktrace && ./configure && make && cp backtrace.h 
/usr/local/include &&
cp .libs/libbacktrace.a /usr/local/lib). Add -lbacktrace to extra-libs. Hacky? 
Yeah...

Derek Buitenhuis (1):
  Allocation NULL check fuzzing tool

 libavutil/mem.c   |  4 ++-
 libavutil/posixmemalign.c | 86 +++
 2 files changed, 89 insertions(+), 1 deletion(-)
 create mode 100644 libavutil/posixmemalign.c

-- 
2.15.0

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

[FFmpeg-devel] [PATCH 1/1][NO NOT APPLY] Allocation NULL check fuzzing tool

[Derek Buitenhuis]

Signed-off-by: Derek Buitenhuis 
---
 libavutil/mem.c   |  4 ++-
 libavutil/posixmemalign.c | 86 +++
 2 files changed, 89 insertions(+), 1 deletion(-)
 create mode 100644 libavutil/posixmemalign.c

diff --git a/libavutil/mem.c b/libavutil/mem.c
index 6ad409daf4..0d9ab3d230 100644
--- a/libavutil/mem.c
+++ b/libavutil/mem.c
@@ -43,6 +43,8 @@
 #include "intreadwrite.h"
 #include "mem.h"
 
+#include "posixmemalign.c"
+
 #ifdef MALLOC_PREFIX
 
 #define malloc AV_JOIN(MALLOC_PREFIX, malloc)
@@ -84,7 +86,7 @@ void *av_malloc(size_t size)
 
 #if HAVE_POSIX_MEMALIGN
 if (size) //OS X on SDK 10.6 has a broken posix_memalign implementation
-if (posix_memalign(&ptr, ALIGN, size))
+if (my_posix_memalign(&ptr, ALIGN, size))
 ptr = NULL;
 #elif HAVE_ALIGNED_MALLOC
 ptr = _aligned_malloc(size, ALIGN);
diff --git a/libavutil/posixmemalign.c b/libavutil/posixmemalign.c
new file mode 100644
index 00..b1970add7b
--- /dev/null
+++ b/libavutil/posixmemalign.c
@@ -0,0 +1,86 @@
+/*
+ * posix_memalign wrapper with random failurres
+ *
+ * Copyright (c) 2013, Derek Buitenhuis
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#define _GNU_SOURCE
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+static void errprint(void *data, const char *msg, int errnum)
+{
+fprintf(stderr, "ERROR: %s.\n", msg);
+}
+
+static int stackprint(void *data, uintptr_t pc, const char *filename, int 
lineno, const char *function)
+{
+fprintf(stderr, "%s:%d in %s\n", function, lineno, filename);
+return 0;
+}
+
+static int my_posix_memalign(void **memptr, size_t alignment, size_t size)
+{
+static time_t   seed  = 0;
+static int  prob  = 0;
+static uint64_t iteration = 0;
+int ret;
+
+struct backtrace_state *state = backtrace_create_state("", 1, errprint, 
NULL);
+
+if (state == NULL)
+abort();
+
+if (!seed) {
+char *usertime = getenv("MALLOC_SEED");
+
+if (!usertime)
+seed = time(NULL);
+else
+seed = atoi(usertime);
+
+srand(seed);
+}
+
+if (!prob) {
+char *userprob = getenv("MALLOC_FAILPROB");
+
+if (!userprob)
+prob = 1;
+else
+prob = atoi(userprob);
+}
+
+if (!(rand() % prob)) {
+fprintf(stderr,
+"\nFAILED. Iteration = %"PRId64", Seed = %lld.\n\n",
+iteration, (long long) seed);
+backtrace_full(state, 0, stackprint, errprint, NULL);
+ret = 0;
+} else {
+ret = posix_memalign(memptr, alignment, size);
+}
+
+iteration++;
+
+return ret;
+}
-- 
2.15.0

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

Re: [FFmpeg-devel] [PATCH 0/1][TOOL][HACK] Allocation NULL check fuzzer

[Derek Buitenhuis]

On 11/24/2017 8:09 PM, Paul B Mahol wrote:
> Do you have backtrace of this one?

Yes, but the alloc failure is not in lavfi:

my_posix_memalign:77 in libavutil/posixmemalign.c
av_malloc:89 in libavutil/mem.c
av_mallocz:240 in libavutil/mem.c
av_packet_alloc:53 in libavcodec/avpacket.c
av_bsf_alloc:106 in libavcodec/bsf.c
bsfs_init:224 in libavcodec/decode.c
avcodec_send_packet:655 in libavcodec/decode.c
decode:2241 in fftools/ffmpeg.c
decode_video:2385 in fftools/ffmpeg.c
process_input_packet:2626 in fftools/ffmpeg.c
process_input:4432 in fftools/ffmpeg.c
transcode_step:4552 in fftools/ffmpeg.c
transcode:4606 in fftools/ffmpeg.c
main:4812 in fftools/ffmpeg.c

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

Re: [FFmpeg-devel] [PATCH 0/1][TOOL][HACK] Allocation NULL check fuzzer

[Derek Buitenhuis]

On 11/24/2017 11:35 PM, Michael Niedermayer wrote:
> Maybe integrating this in:
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> 
> would make sense
> 
> That would run it automatically on ffmpeg master HEAD on powerfull hw

Could make sense, yeah - wouldn't be that hard.

It would probably make an absolute ton of reports, since there are quite
a few unchecked allocs in FFmpeg... might be kinda spammy.

Also not sure how Google feels about using oss-fuzz to look for missing
NULL checks? Is there some set of guidelines?

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

Re: [FFmpeg-devel] [PATCH] lavr: deprecate the entire library

[Derek Buitenhuis]

On 11/25/2017 2:40 PM, Clément Bœsch wrote:
> Maybe "libavresample is not maintained by the FFmpeg project and will be
> dropped at the next major bump. Please use libswresample instead."
> 
> And it probably needs a longer explanation somewhere (website/news/...)

All API functions should be properly marked as deprecated in the headers.

Relying on a stderr warning to help migrate API users is silly, who is
going to be watching stderr using a /library/?

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

Re: [FFmpeg-devel] [PATCH 0/1][TOOL][HACK] Allocation NULL check fuzzer

[Derek Buitenhuis]

On 11/26/2017 12:14 AM, Carl Eugen Hoyos wrote:
> I am of course in favour of such checks but is there an allocator we support
> that actually returns NULL on oom?

Anything that doesn't use overcommit. Windows is the big obvious one here. Also
various UNIX-like things, and even Linux is not guaranteed to return non-NULL,
depending on how the kernel is set up (e.g. on some RHELs I think, or on
plenty of embedded setups.) 

Some libcs will fail if the requested size is outside of the allowed range.

In any case, the checks should be done.

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

Re: [FFmpeg-devel] [PATCH 2/2] vorbisenc: Check the return value of av_frame_clone

[Derek Buitenhuis]

On 11/24/2017 7:27 PM, Derek Buitenhuis wrote:
> Prevents a segfault when alloc fails.
> 
> Signed-off-by: Derek Buitenhuis 
> ---
>  libavcodec/vorbisenc.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

If there are no objections, I'll push this today after a FATE run.

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

Re: [FFmpeg-devel] [PATCH 3/3] udp: Actually fail when we're missing required options, like the "warning" says.

[Derek Buitenhuis]

On 11/22/2017 3:28 PM, Derek Buitenhuis wrote:
> Signed-off-by: Derek Buitenhuis 
> ---
> There was no reasoning in the commit that added this, so maybe someone on
> the list has some insights.
> ---
>  libavformat/udp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Ping.

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

Re: [FFmpeg-devel] [PATCH]configure: libvmaf depends on pthreads

[Derek Buitenhuis]

On 11/26/2017 1:35 PM, Carl Eugen Hoyos wrote:
> Attached patch adds a missing dependency to libvmaf, I don't know if
> other threads also work.

This should also be filed as a bug against libvmaf, since its pkg-config
file isn't complete, then.

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

Re: [FFmpeg-devel] [PATCH]configure: libvmaf depends on pthreads

[Derek Buitenhuis]

On 11/26/2017 1:50 PM, Carl Eugen Hoyos wrote:
> Sorry, I don't understand how pkg-config is related to the missing
> dependency of our configure script: Please explain.

Because pthreads is a dependency of libvmaf.

Looking at libvmaf, it does list pthreads as a dependency:

https://github.com/Netflix/vmaf/blob/master/wrapper/libvmaf.pc

It should work as-is. Is there a way I can reproduce this?

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

Re: [FFmpeg-devel] [PATCH]configure: libvmaf depends on pthreads

[Derek Buitenhuis]

On 11/26/2017 2:02 PM, Carl Eugen Hoyos wrote:
> The way I understand (Linux) dynamic libraries, one implies
> the other.

The problem is that libvmaf's .pc file put all of its deps in
Libs instead of splitting them out into Libs.private, which
is used only when static linking. Stuff like -lpthreads is
only needed if static linking, and stuff like -lstdc++ is
just wrong on any system that doesn't use libstdc++ (and
also only used for static linking).

I assume this is what Nicholas is referring to.

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

Re: [FFmpeg-devel] [PATCH]configure: libvmaf depends on pthreads

[Derek Buitenhuis]

On 11/26/2017 2:05 PM, Nicolas George wrote:
> If a library does not use threads but our calls to the library require
> threads, then we must consider it a dependency.

Netflix made their pkg-config file incorrectly, which causes this. It should
be fixed there, but do we want to work around that in the mean time like this?
I have no real strong opinion on that, I just thought it should be reported
upstream so they could fix it.

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

Re: [FFmpeg-devel] [PATCH]configure: libvmaf depends on pthreads

[Derek Buitenhuis]

On 11/26/2017 2:06 PM, Carl Eugen Hoyos wrote:
> In this case the library and our interface both depend on pthreads
> but configure ignores this.

Sorry, I wasn't aware our own wrapper code use pthreads too. Patch should
be OK then.

Upstream pkg-config file is still broken, though. :)

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

Re: [FFmpeg-devel] [PATCH]configure: libvmaf depends on pthreads

[Derek Buitenhuis]

On 11/26/2017 2:09 PM, Carl Eugen Hoyos wrote:
> As said before, it is non-trivial to find such a system
> (it worked fine here on osx when I tested last).

OS X does not ship with libstdc++, IIRC. You must have been using a
ports-build toolchain? Using clang-cl or MSVC on windows will also not
use libstdc++. FreeBSD does not use libstdc++.

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

Re: [FFmpeg-devel] [PATCH]configure: libvmaf depends on pthreads

[Derek Buitenhuis]

On 11/26/2017 2:10 PM, Nicolas George wrote:
> -> our code depends on pthreads for this filters, it must be expressed
> in configure: Carl Eugen's patch is right, there is no need to bugreport
> anything. His explanations later were wrong, but it may only be caused
> by the confusion you brought.

Yeah I didn't realize that. Apologies on that.

> 
> What you describe is possibly true, but not related.

Correct - I will still report it upstream.

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

Re: [FFmpeg-devel] [PATCH]configure: libvmaf depends on pthreads

[Derek Buitenhuis]

On 11/26/2017 2:15 PM, Carl Eugen Hoyos wrote:
> I believe I have explained before that I always only use
> vanilla toolchains because that's the only thing users
> typically have access to.

It's possible the OS X version was outdated then, since the
system clang has used libc++ as default since OS X 10.9.

But I digress, this is now off-topic, and no longer related to
the patch at hand.

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

Re: [FFmpeg-devel] [PATCH 0/1][TOOL][HACK] Allocation NULL check fuzzer

[Derek Buitenhuis]

On 11/25/2017 12:07 AM, Michael Niedermayer wrote:
> I do not know that but i would be surprised if null dereferences tests
> where unwelcome
> 
> oss-fuzz will already report null derferences and OOM conditions, as
> well as undefined behavior. So in some sense various points on the map
> surrounding this here are already tested for

Locally, I've made this work with something like:

configure --malloc-prefix=fuzzer_ --extra-libs=-lallocfuzz

I'll push that library up to a git repo some time today.

Should be pretty easy to integrate into oss-fuzz like this, I think?

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

Re: [FFmpeg-devel] [PATCH] lavf/mov: fix huge alloc in mov_read_ctts

[Derek Buitenhuis]

On 11/26/2017 3:10 PM, John Stebbins wrote:
> Is there some git magic for this, or is this just something you add manually 
> to the bottom of the commit message?

It's just manual.

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

Re: [FFmpeg-devel] [ogm] Free extradata before reallocating.

[Derek Buitenhuis]

On 11/21/2017 11:12 PM, Dale Curtis wrote:
> Otherwise ff_alloc_extradata() just leaks any existing allocated
> memory.

Should be OK.

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

Re: [FFmpeg-devel] [PATCH 5/6] Add suppoort for using libklvanc from within decklink capture module

[Derek Buitenhuis]

On 11/29/2017 7:17 PM, Devin Heitmueller wrote:
>> Is there a reason we shouldn't fail hard here?
> 
> Not really.  The parser will log an error if the callback returns a nonzero 
> value, but beyond the return value isn’t actively used.  That said, no 
> objection to having it return -1 for clarity.

I have no strong feelings either way.

>> I thought C++ didn't have designated initializers? Maybe my C++ is rusty.
> 
> Clang didn’t complain, and g++ only complains if you put them in a 
> non-default order (i.e. "non-trivial designated initializers not supported"). 
>  The designated initializers improve readability but aren’t required (since 
> already put the items in the default order).  If there’s a portability 
> concern then I can get rid of them.

The internet seems to claim it's a GNU extension. Will this code ever possibly
be built with something that isn't GCC or Clang?

>> Same for other occurrences.
> 
> I’m sorry, but what other occurrences?  I don’t see any other instances in 
> this patch where designated initializers are used — or did I misunderstand 
> your comment?

My mail must have got mangled while I was editing it. Ignore this, I think.

>> Is buf guaranteed to be properly aligned for this, or will cause aliasing 
>> problems?
> 
> Hmm, good question.  The start of each line will always be aligned on a 48 
> byte boundary as a result of how the decklink module manages it’s buffers, 
> but I agree that this block of code is a bit messy and needs some cleanup 
> (hence the TODO).

Is this aligment a guarantee by the module?

> I suspect the original routine was cribbed from OBE (with portions derived 
> from ffmpeg’s v210dec), and the assembly version of the same function 
> probably isn’t as forgiving (although libklvanc doesn’t provide an assembly 
> implementation as this routine isn’t particularly performance sensitive).

[...]

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

Re: [FFmpeg-devel] [PATCH] libavformat/dashdec: Fix for ticket 6658 (Dash demuxer segfault)

[Derek Buitenhuis]

On 12/4/2017 4:28 AM, Colin NG wrote:
> ---
>  libavformat/dashdec.c | 112 
> --
>  1 file changed, 99 insertions(+), 13 deletions(-)

Please describe what is actually being changed, and why, in the
commit message. It is both hard to review with no description,
and incredibly annoying to git blame later, without a proper
commit message. For example, a bunch of these changes seem
pretty disparate, hence my 'Why?' after several.

> +static char * ishttp(char *url) {
> +char *proto_name = avio_find_protocol_name(url);
> +return av_strstart(proto_name, "http", NULL);
> +}

Is the URL guaranteed to have a known, and enabled (in avforma) protocol?
If not, then this can crash, because avio_find_procotol_name will return
NULL.

> -ret = s->io_open(s, pb, url, AVIO_FLAG_READ, &tmp);
> +av_freep(pb);
> +ret = avio_open2(pb, url, AVIO_FLAG_READ, c->interrupt_callback, &tmp);

Why?
 
> +static int resolve_content_path(AVFormatContext *s, const char *url,  
> xmlNodePtr *baseurl_nodes,  int n_baseurl_nodes) {
> +
> +int i;
> +char *text;
> +char *tmp_str = av_mallocz(MAX_URL_SIZE);
> +char *tmp_str_2= av_mallocz(MAX_URL_SIZE);
> +
> +char *path = av_mallocz(MAX_URL_SIZE);

If the size is known (and not massive), why are these heap allocations?

> +int nameSize = 0;
> +int updated = 0;
> +
> +if (!tmp_str || !tmp_str_2 || !path) {
> +updated = AVERROR(ENOMEM);
> +goto end;
> +}
> +
> +av_strlcpy(tmp_str, url, strlen(url)+1);
> +char *mpdName = strtok (tmp_str," /");

Don't mix declarations and code.

> +
> +while ((mpdName =strtok (NULL, "/"))) {
> +nameSize = strlen(mpdName);
> +}
> +
> +av_strlcpy (path, url, strlen(url)-nameSize+1);
> +
> +int rootId = 0;
> +xmlNodePtr  *node = NULL;

Ditto.

> +for (rootId = n_baseurl_nodes-1; rootId >0; rootId--) {
> +if (!(node = baseurl_nodes[rootId])) {
> +continue;
> +}
> +if (ishttp(xmlNodeGetContent(node))) {
> +break;
> +}
> +}
> +
> +node = baseurl_nodes[rootId];
> +char *baseurl = xmlNodeGetContent(node);
> +char *root_url = (!av_strcasecmp(baseurl, ""))? path: baseurl;

Ditto.

Also should all of these calls to the XML lib have checks? My gut says 'yes'.

> +
> +if (node) {
> +xmlNodeSetContent(node, root_url);
> +}
> +
> +int size = strlen(root_url);
> +char *isRootHttp= ishttp(root_url);
> +
> +char token ='/';

Ditto.

> +//if (root_url[size-1]==token) {
> +if (av_strncasecmp(&root_url[size-1],&token, 1) != 0) {
> +av_strlcat(root_url, "/", size+2);
> +size+=2;
> +}
> +
> +for (i = 0; i < n_baseurl_nodes; ++i) {
> +if (i==rootId) {
> +continue;
> +}
> +text = xmlNodeGetContent(baseurl_nodes[i]);
> +if (text) {
> +memset(tmp_str, 0, strlen(tmp_str));
> +
> +if (!ishttp(text) && isRootHttp) {
> +av_strlcpy(tmp_str, root_url, size+1);
> +}
> +int start = (text[0]==token) ? 1: 0;

Mixed code and variable declarations. Also, redundant ternary operation.

> +memset(tmp_str_2, 0, strlen(tmp_str_2));
> +av_strlcat(tmp_str, text+start, MAX_URL_SIZE);
> +xmlFree(text);
> +xmlNodeSetContent(baseurl_nodes[i], tmp_str);
> +updated = 1;

What's with the odd 0/1 return values, which are not even
checked anywhere?

> +}
> +}
> +
> +end:
> +av_free(path);
> +av_free(tmp_str);
> +av_free(tmp_str_2);
> +return updated;
> +
> +}
>  static int parse_manifest_representation(AVFormatContext *s, const char *url,
>   xmlNodePtr node,
>   xmlNodePtr adaptionset_node,
> @@ -698,6 +786,12 @@ static int parse_manifest_representation(AVFormatContext 
> *s, const char *url,
>  baseurl_nodes[2] = adaptionset_baseurl_node;
>  baseurl_nodes[3] = representation_baseurl_node;
>  
> +ret = resolve_content_path(s, url, baseurl_nodes, 4);
> +
> +if (ret == AVERROR(ENOMEM)) {
> +goto end;
> +}

This kind of check seems very wrong. Check for ret < 0.

>  mpd_baseurl_node = find_child_node_by_name(node, "BaseURL");
> +if (!mpd_baseurl_node) {
> +mpd_baseurl_node = xmlNewNode(node, "BaseURL");
> +}

Why? Also missing check for xmlNewNode ret value?

>  
>  // at now we can handle only one period, with the longest duration
>  node = xmlFirstElementChild(node);
> @@ -1315,6 +1412,7 @@ static int read_from_url(struct representation *pls, 
> struct fragment *seg,
>  } else {
>  ret = avio_read(pls->input, buf, buf_size);
>  }
> +
>  if (ret > 0)

Stray change.

> -/* Seek to the requested position. If this was a HTTP request, the of

Re: [FFmpeg-devel] [PATCH] fix MSVC compilation errors

[Derek Buitenhuis]

On 12/4/2017 8:03 AM, Mateusz wrote:
> After commit 3701d49 'error_resilience: remove avpriv_atomic usage'
> we have included windows.h in much more files and we should
> avoid conflicts with defines/function declarations.
> 
> Signed-off-by: Mateusz Brzostek 
> ---
>  libavcodec/jpegls.h  | 4 
>  libavcodec/mss2.c| 6 +++---
>  libavformat/mxfenc.c | 2 +-
>  3 files changed, 8 insertions(+), 4 deletions(-)

Sprinkling these weird ifdefs and renames around is pretty ugly. Is there
some sort of canonical list on MSDN or something we can use globally-ish?

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

Re: [FFmpeg-devel] [PATCH] Refactor Developer Docs, update dev list section (v2)

[Derek Buitenhuis]

On 12/4/2017 12:45 PM, Carl Eugen Hoyos wrote:
> Committers have to be subscribed to -cvslog.

"Have to"? I certainly am not, and neither are many. Are you going to
kick all of them out and revoke their push access? I think not.

It's clear that the majority here do not agree with you on this issue,
in my opinion. Either put it to a vote or stop the needless, and endless
bikshedding on this simple documentation patch.

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

Re: [FFmpeg-devel] [PATCH] avcodec/libx265.c - Add named option to set profile

[Derek Buitenhuis]

On 12/5/2017 9:22 AM, Gyan Doshi wrote:
> Revised patch attached.

[...]

> From bbb8013e7404360139a13b58a377a29d3ca69552 Mon Sep 17 00:00:00 2001
> From: Gyan Doshi 
> Date: Tue, 5 Dec 2017 13:17:53 +0530
> Subject: [PATCH] avcodec/libx265 - Add named option to set profile
> 
> Adds call to x265_param_apply_profile after x265_param_parse.
> Added as private option since HEVC profiles other than
> Main, Main 10 and MSP in AVCodecContext are consolidated in a single
> constant.

This is inconsistent with the way libx264.c does it. It calls the profile
function before the param parsing.

> +if (ctx->profile) {
> +if (ctx->api->param_apply_profile(ctx->params, ctx->profile) < 0) {
> +av_log(avctx, AV_LOG_ERROR, "Invalid or incompatible profile set 
> : %s.\n", ctx->profile);
> +av_log(avctx, AV_LOG_INFO, "Profile must match bit depth and 
> chroma sampling of output stream.\n");

Drop the second log line.

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

Re: [FFmpeg-devel] [PATCH] tests/fate/mov: Disable fate-mov-invalid-elst-entry-count, the test does not work reliable currently

[Derek Buitenhuis]

On 12/5/2017 12:38 AM, Michael Niedermayer wrote:
> Noone is known to work on fixing this, so it should be disabled
> 
> Signed-off-by: Michael Niedermayer 
> ---
>  tests/fate/mov.mak | 1 -
>  1 file changed, 1 deletion(-)

*NAK*

Disabling failing tests entirely defeats the point of having test!

The commit that broke it should be reverted until the author
of that commit can explain why it changed, or fix it.

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

Re: [FFmpeg-devel] [PATCH] avcodec/libx265.c - Add named option to set profile

[Derek Buitenhuis]

On 12/5/2017 2:05 PM, Gyan Doshi wrote:
> 
> On 12/5/2017 7:21 PM, Derek Buitenhuis wrote:
> 
>> This is inconsistent with the way libx264.c does it. It calls the profile
>> function before the param parsing.
> 
>  From http://x265.readthedocs.io/en/default/cli.html#profile-level-tier
> 
> "API users must call x265_param_apply_profile() after configuring their 
> param structure. Any changes made to the param structure after this call 
> might make the encode non-compliant."
> 
> and
> 
> "Also note that x265 determines the decoder requirement profile and 
> level in three steps. First, the user configures an x265_param structure 
> with their suggested encoder options and then optionally calls 
> x265_param_apply_profile() to enforce a specific profile (main, main10, 
> etc). "

Sounds like a very good reason to me, then.

Patch OK with log line dropped.

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

Re: [FFmpeg-devel] [PATCH] tests/fate/mov: Disable fate-mov-invalid-elst-entry-count, the test does not work reliable currently

[Derek Buitenhuis]

>> The commit that broke it should be reverted until the author
>> of that commit can explain why it changed, or fix it.
> 
> The commit that added the test was the one that broke fate. It never
> worked.
> So this "sort of" reverts what caused the issue.

Wasn't the code it tests added directly before the commit that added this
test? That's the code that is broken.

The way I see it, the code is  workaround for broken files, but it doesn't
actually work. It should either be fixed, or the workaround removed if
nobody (especially the author) is willing to fix it.

> Ill make this more clear in the commit message in case you otherwise
> agree to the change ?
> 
> I can also exactly revert
> the commit that added the test if thats preferred?

See above.

As I stated before, the entire point of tests is to show something is broken.
They make FATE red so someone will fix it. Disabling the test demeans the
entire point of having tests - if nobody is willing to fix the broken code
it tests, that broken could should be removed, or not have been committed.

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

Re: [FFmpeg-devel] [RFC] avcodec/avcodec.h: Add encryption info side data

[Derek Buitenhuis]

On 12/5/2017 11:00 PM, Jacob Trimble wrote:
> Also, can I use the flexible array member feature, it was introduced
> in C99?  Would a 0-length array be better?

No, I don't think this would be OK inside a public header, unfortunately.

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

Re: [FFmpeg-devel] [PATCH] tests/fate/mov: Disable fate-mov-invalid-elst-entry-count, the test does not work reliable currently

[Derek Buitenhuis]

On 12/6/2017 12:12 AM, Michael Niedermayer wrote:
> The test produces different output on qemu arm and x86-64.
> From this we know there is a bug, but not where the bug is.
> It can be in the test, the newly added code tested or code that was
> there before.
> 
> My guess was, its the test, i cannot logically explain why.
> 
> ive looked into this now and its missing -idct, adding that makes it
> produce the same result here
> 
> ill push a fix for this

Thank you for taking the time to look into it.

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

Re: [FFmpeg-devel] [RFC] avcodec/avcodec.h: Add encryption info side data

[Derek Buitenhuis]

On 12/6/2017 12:36 AM, Jacob Trimble wrote:
> Would a 0-length array work?  Otherwise I would need to have it be a
> 1-length array and have to account for that when calculating the size
> to allocate; it would also require a comment to ignore the size of the
> array.

Aren't 0-length arrays a GNU extensions? If so, I would gather that it
probably is not OK in a public header, either.

> The reason I want it to be an array is because I want a variable
> number of elements, but I want the data contained in the struct.
> Since this will be extra data, AFAIK it will only be free()'d, so I
> can't use pointers or it will leak.
> 
> Another alternative would be to still malloc more than needed and have
> the memory past the struct be the array.  That seems like a hack, but
> would allow a simple free().  For example:

I'm not entirely sure what way we prefer nowadays, but you can see
we've had side data with variable length members before with e.g.
AV_PKT_DATA_QUALITY_STATS, but that doesn't have a struct associated
with it. I'm hoping someone more up to date with the side data stuff
can chime in with a suggestion on what our current best practices
are for it.

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

Re: [FFmpeg-devel] [PATCH 0/1][TOOL][HACK] Allocation NULL check fuzzer

[Derek Buitenhuis]

On 11/25/2017 12:07 AM, Michael Niedermayer wrote:
> I do not know that but i would be surprised if null dereferences tests
> where unwelcome
> 
> oss-fuzz will already report null derferences and OOM conditions, as
> well as undefined behavior. So in some sense various points on the map
> surrounding this here are already tested for

https://github.com/dwbuiten/nullfuzzer

Works without patching FFmpeg now, and is thread-safe. Haven't put it into
oss-fuzz yet.

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

Re: [FFmpeg-devel] [RFC] [Vote] Drop Windows XP support

[Derek Buitenhuis]

On 12/14/2017 1:26 PM, wm4 wrote:
> The subject of the vote is:
> 
>   Should we drop support for Windows XP starting in git master and the
>   next FFmpeg major release?

I am not on the voting list (which is form 2015 and contains people who have
since vanished, as well), but I would like to put forth my support for 'Yes',
even if it doesn't count as a real vote.

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

Re: [FFmpeg-devel] [PATCH] avcodec/libx264: fix compilation with x264 builds >= 153

[Derek Buitenhuis]

On 12/25/2017 8:58 PM, James Almer wrote:
> @@ -272,6 +272,7 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, 
> const AVFrame *frame,
>int *got_packet)
>  {
>  X264Context *x4 = ctx->priv_data;
> +const av_unused AVPixFmtDescriptor *desc = 
> av_pix_fmt_desc_get(ctx->pix_fmt);

Why is this marked unused? Its usage is not behind any ifdef.

>  x264_nal_t *nal;
>  int nnal, i, ret;
>  x264_picture_t pic_out = {0};
> @@ -279,7 +280,7 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, 
> const AVFrame *frame,
>  
>  x264_picture_init( &x4->pic );
>  x4->pic.img.i_csp   = x4->params.i_csp;
> -if (x264_bit_depth > 8)
> +if (desc->comp[0].depth > 8)
>  x4->pic.img.i_csp |= X264_CSP_HIGH_DEPTH;

Should this and the previous part be part of a different commit? They seem
more like a bugfix than an API usage change.

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

Re: [FFmpeg-devel] [PATCH] avcodec/libx264: fix compilation with x264 builds >= 153

[Derek Buitenhuis]

On 12/25/2017 9:22 PM, James Almer wrote:
> Want me to change it in a separate commit?

Sure.

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

Re: [FFmpeg-devel] [PATCH] lavr: deprecate the entire library

[Derek Buitenhuis]

On 12/26/2017 3:54 PM, Rostislav Pehlivanov wrote:
> This has been on the ML for over a month. No one disagreed then. No one was
> on vacation then.

I agree with Nicholas, sending a new, different version of a patch, *on 
Christmas Day*,
and pushing on Boxing Day if nobody has objections, is not OK. It's sketchy as 
hell.

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

Re: [FFmpeg-devel] [PATCH] lavr: deprecate the entire library

[Derek Buitenhuis]

On 12/26/2017 4:10 PM, Nicolas George wrote:
> I missed that. If the proposal was similar enough to what was sent
> yesterday, then my objection does not stand, of course, sorry.

The version on the ML then was different, and it never reached consensus at
all on wording. I object for the reasons stated in my other mail.

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

Re: [FFmpeg-devel] [PATCH] lavr: deprecate the entire library

[Derek Buitenhuis]

On 12/26/2017 4:52 PM, Rostislav Pehlivanov wrote:
> You didn't even bother to read the patch? Your objections have been
> addressed.
> Since your new objection is invalid, I still intent to push it.

I said wording, which others had issues with, and were *not* addressed until
v2 of the patch yesterday. On Christmas day. I do not object to the patch,
but I massively object to the way it's being forced through over Christmas,
with very little time, when many are with their e.g. families.

> Either give a valid objection or I'm pushing tomorrow night.

Threat-style "give me an objection NOW or I'll push!!" over the holidays.
Delightful. Very professional.

It would be nice if this community moved away from the "aggressive asshole"
style of development, as seen here, but after so many years, that just
seems impossible. Normal people get pushed away by aggressive assholes.

A very warm and welcoming community we have here.

Happy Holidays!

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

Re: [FFmpeg-devel] [PATCH] lavr: deprecate the entire library

[Derek Buitenhuis]

On 12/26/2017 5:03 PM, Nicolas George wrote:
> Derek Buitenhuis (2017-12-26):
>> It would be nice if this community moved away from the "aggressive asshole"
>> style of development, as seen here, but after so many years, that just
>> seems impossible. Normal people get pushed away by aggressive assholes.
> 
> I agree on the idea. Maybe start by not calling people "aggressive
> assholes"? ;-)

Very good point; I wrote that in frustration.

I apologize.

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

Re: [FFmpeg-devel] [PATCH] avfilter/vf_tonemap: don't use NAN constant as an initializer

[Derek Buitenhuis]

On 9/8/2017 10:53 AM, Hendrik Leppkes wrote:
> So if it would not be defined at all, the filter would still not
> build. So what exactly does that change?

Yes it would:

libavutil/mathematics.h:#define NANav_int2float(0x7fc0)

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

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/scpr: optimize shift loop.

[Derek Buitenhuis]

On 9/8/2017 11:15 PM, James Almer wrote:
> It reads eight bytes at a time if the buffer is sufficiently aligned,
> then finishes reading the remaining bytes one at a time.
> If the buffer is unaligned, it reads everything one byte at a time like
> it used to.
> 
> See ff_h2645_extract_rbsp() and add_bytes_c() for another example of
> this optimization.

So put a comment, or at least put it in the commit message.

It isn't exactly straightforward; it's like reading Hex Rays output.

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

Re: [FFmpeg-devel] [PATCH] configure: check if NAN can be used as a constant initializer

[Derek Buitenhuis]

On 9/13/2017 8:39 PM, James Almer wrote:
> Maybe not, but I'm not going to spend time reading and testing the code
> to find out if that's the case when the maintainer hasn't dealt with
> this himself for an entire month.

He's on vacation, I think.

> As i said, if anyone has a better or preferred solution I'll not oppose
> a patch implementing it and reverting mine.

I prefer this one, because, for example, it covers zscale, too, which
can't be fixed by using DBL_MAX/DBL_MIN, since NAN is part of libzimg's
public API.

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

Re: [FFmpeg-devel] [PATCH] pixdesc: Add API to map color property name to enum value

[Derek Buitenhuis]

On 9/20/2017 7:30 PM, wm4 wrote:
> To be honest, ENOSYS is even more confusing than -1. Think about what
> happens if someone converts the error to a string and displays that to
> a user.

AVERROR(EINVAL)?

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

Re: [FFmpeg-devel] [PATCH]lavc/utvideo: Use "&" instead of "&&" in expressions with "~"

[Derek Buitenhuis]

On 10/8/2017 12:58 AM, Ronald S. Bultje wrote:
> I personally think the warning is dumb... But I guess that's just me.

I agree; I don't even count it as a valid warning. Maybe disable it?

That's only my opinion, of course.

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

Re: [FFmpeg-devel] [PATCH] Add support for libopenjpeg 2.3

[Derek Buitenhuis]

On 10/8/2017 4:18 PM, Michael Bradshaw wrote:
> I can clean these up as part of the patch that drops OpenJPEG 1.x support,
> which I plan on making after the next release (though of course someone
> else is welcome to beat me to it; it seems there's a race for OpenJPEG
> patches!).

Is there anything that precludes switching 2.X to pkg-config and leaving
1.X as is?

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

Re: [FFmpeg-devel] [PATCH 2/6] vaapi: Remove H.264 baseline profile

[Derek Buitenhuis]

On 10/8/2017 4:11 PM, Mark Thompson wrote:
> +case FF_PROFILE_H264_BASELINE:
> +// Baseline profile is not supported, assume the user meant
> +// constrained baseline instead.
> +avctx->profile = FF_PROFILE_H264_CONSTRAINED_BASELINE;

Trying to automatically (and silently!) guess what the user wanted
is never a good idea, IMO. At the very least, print a warning.

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

Re: [FFmpeg-devel] [PATCH 3/6] vaapi: Always free parameter buffers after vaEndPicture() with libva2

[Derek Buitenhuis]

On 10/8/2017 4:11 PM, Mark Thompson wrote:
> This is an ABI change in libva2: previously the Intel driver had this
> behaviour and it was implemented as a driver quirk, but now it is part
> of the specification so all drivers must do it.
> ---
>  libavcodec/vaapi_decode.c  | 4 ++--
>  libavcodec/vaapi_encode.c  | 4 ++--
>  libavfilter/vf_deinterlace_vaapi.c | 2 +-
>  libavfilter/vf_scale_vaapi.c   | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)

Seems correct to me.

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

Re: [FFmpeg-devel] [PATCH 5/6] hwcontext: Perform usual initialisation on derived device contexts

[Derek Buitenhuis]

On 10/8/2017 4:11 PM, Mark Thompson wrote:
> -ret = qsv_device_init(ctx);
> -if (ret < 0)
> -goto fail;

From the patch context alone, this looks kinda iffy. I assume
qsv_device_init is now called via av_hwdevice_ctx_init?

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

Re: [FFmpeg-devel] [PATCH 4/6] hwcontext_vaapi: Set message callbacks on internally-created devices

[Derek Buitenhuis]

On 10/8/2017 4:11 PM, Mark Thompson wrote:
> The message callbacks are library-safe in libva2, so we can now use
> them.  Also factorise out the common connection code to avoid
> duplicating this change.
> ---
>  libavutil/hwcontext_vaapi.c | 74 
> +++--
>  1 file changed, 45 insertions(+), 29 deletions(-)

Would have preferred these be two separate patches, but I also don't
have strong feelings about it.

Looks OK, assuming libva2 guarantees NULL-termination of the message.

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

Re: [FFmpeg-devel] [PATCH 2/6] vaapi: Remove H.264 baseline profile

[Derek Buitenhuis]

On 10/8/2017 4:49 PM, Mark Thompson wrote:
> Yeah, ok, I agree.  Patch changed as enclosing.
> 
> 
>  libavcodec/vaapi_decode.c  |  1 -
>  libavcodec/vaapi_encode_h264.c | 12 
>  2 files changed, 4 insertions(+), 9 deletions(-)

Looks OK to me. I assume we don't care about the old lib
versions and hardware wanting to use the profile.

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

Re: [FFmpeg-devel] [PATCH 5/6] hwcontext: Perform usual initialisation on derived device contexts

[Derek Buitenhuis]

On 10/8/2017 4:52 PM, Mark Thompson wrote:
> Yes.  It always was for the non-derived case, resulting in it being called 
> twice (for qsv it doesn't actually do anything beyond some checks, so this 
> was harmless).

Looks OK, then.

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

Re: [FFmpeg-devel] [PATCH 6/6] hwcontext_vaapi: Add support for mapping to DRM objects

[Derek Buitenhuis]

On 10/8/2017 4:11 PM, Mark Thompson wrote:
> Uses vaExportSurfaceHandle() from libva2.
> ---
>  libavutil/hwcontext_vaapi.c | 106 
> +++-
>  1 file changed, 104 insertions(+), 2 deletions(-)

[...]

> +for (i = 0; i < drm_desc->nb_objects; i++)
> +close(drm_desc->objects[i].fd);

Delightful API...

> +surface_id = (VASurfaceID)(uintptr_t)src->data[3];

Can you elaborate a bit on this part? Casting pointers to uintptr_t and storing
them is always a red flag to me... C standard issues, etc.

> +dst->data[0] = (uint8_t*)drm_desc;

This is also a bit wtf-looking... is it cast back at some point? That could be
problematic.

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

Re: [FFmpeg-devel] [PATCH 4/6] hwcontext_vaapi: Set message callbacks on internally-created devices

[Derek Buitenhuis]

On 10/8/2017 5:00 PM, Mark Thompson wrote:
> Easy enough to do.
> 
> Split as:
> 
> """
> hwcontext_vaapi: Factorise out common connection code
> 
> This was duplicated between normal device creation and creation by
> derivation from a DRM device.
> """
> 
> and
> 
> """
> hwcontext_vaapi: Set message callbacks on internally-created devices
> 
> The message callbacks are library-safe in libva2, so we can now use
> them.
> """
> 
> with the obvious change.

Sounds good.

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

Re: [FFmpeg-devel] [PATCH 6/6] hwcontext_vaapi: Add support for mapping to DRM objects

[Derek Buitenhuis]

On 10/8/2017 5:11 PM, Mark Thompson wrote:
> This is just how hardware surfaces are stored in AVFrames - they have their 
> own API-specific handles in the data[] pointers because that's the only place 
> to put them.
> 
> See
> 
> and
> 
> (and others).
> 
> Thanks,

Eugh, well OK. That's arguably not exactly OK by the C standard. Oh well.

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

Re: [FFmpeg-devel] [PATCH] avformat/srt: add Haivision Open SRT protocol

[Derek Buitenhuis]

On 10/10/2017 7:29 AM, Nablet Developer wrote:
> @@ -293,6 +293,7 @@ External library support:
>--enable-opengl  enable OpenGL rendering [no]
>--enable-openssl enable openssl, needed for https support
> if gnutls is not used [no]
> +  --enable-opensrt enable Haivision Open SRT protoco [no]

Usually we have this in the form of --enable-libXXX for external libs. OpenSSL
and stuff doesn't follow this for reasons I do not know... maybe someone knows
if we have a real rule set or (more likely) it's just random.

>  enabled omx   && require_header OMX_Core.h
> +enabled opensrt   && require_pkg_config libsrt srt srt/srt.h 
> srt_socket

Is there a >= version required?

> +#define _DEFAULT_SOURCE
> +#define _BSD_SOURCE /* Needed for using struct ip_mreq with recent glibc 
> */

These should be passed as -D_THING to the compiler, normally, no?
> +#ifdef __APPLE__
> +#include "TargetConditionals.h"
> +#endif

What is this and why is it with "" instead of <>? Google says it is a system 
header,
so "" is wrong.

> +#ifndef HAVE_PTHREAD_CANCEL
> +#define HAVE_PTHREAD_CANCEL 0
> +#endif

This looks terribly wrong. Why is this needed?

> +#ifndef IPV6_ADD_MEMBERSHIP
> +#define IPV6_ADD_MEMBERSHIP IPV6_JOIN_GROUP
> +#define IPV6_DROP_MEMBERSHIP IPV6_LEAVE_GROUP
> +#endif

At the very list this needs a comment to explain why.

> +#define UDP_TX_BUF_SIZE 32768
> +#define UDP_MAX_PKT_SIZE 65536
> +#define UDP_HEADER_SIZE 8

Ditto.

> +#if HAVE_PTHREAD_CANCEL
> +pthread_t circular_buffer_thread;
> +pthread_mutex_t mutex;
> +pthread_cond_t cond;
> +int thread_started;
> +#endif

Am I mistaken in thinking this code is pretty iffy when pthread_cancel is not 
available?

> +uint8_t tmp[UDP_MAX_PKT_SIZE+4];

+4 ?

> +/* SRT socket options (srt/srt.h) */
> +int64_t maxbw;
> +int pbkeylen;
> +char passphrase[65];

65?

> +int mss;
> +int fc;
> +int ipttl;
> +int iptos;
> +int64_t inputbw;
> +int64_t oheadbw;
> +int tsbpddelay;
> +int tlpktdrop;
> +int nakreport;
> +int conntimeo;

Why do variable names suddenly change from like_this jammedtogetherlikethis?

> +#define SRT_MODE_CALLER 0
> +#define SRT_MODE_LISTENER 1
> +#define SRT_MODE_RENDEZVOUS 2

enum?

> +{ "buffer_size","System data size (in bytes)", 
> OFFSET(buffer_size),AV_OPT_TYPE_INT,{ .i64 = -1 },-1, INT_MAX, 
> .flags = D|E },

What is system data size, and can it be detected?

> +{ "localport",  "Local port",  
> OFFSET(local_port), AV_OPT_TYPE_INT,{ .i64 = -1 },-1, INT_MAX, 
> D|E },
> +{ "local_port", "Local port",  
> OFFSET(local_port), AV_OPT_TYPE_INT,{ .i64 = -1 },-1, 

... what?

> +{ "reuse",  "explicitly allow reusing UDP sockets",
> OFFSET(reuse_socket),   AV_OPT_TYPE_BOOL,   { .i64 = -1 },-1, 1,   
> D|E },
> +{ "reuse_socket",   "explicitly allow reusing UDP sockets",
> OFFSET(reuse_socket),   AV_OPT_TYPE_BOOL,   { .i64 = -1 },-1, 1,   
> .flags = D|E },

These duplicates are not acceptable.

> +/* SRT socket options (srt/srt.h), see srt/common/socketoptions.hpp */

Is this even a public header? Does libsrt not have real documentation?

> +{ "passphrase", "Crypto PBKDF2 Passphrase size[0,10..64] 0:disable 
> crypto",OFFSET(passphrase), AV_OPT_TYPE_STRING, { .str = 
> NULL },  .flags = D|E },
> +{ "mss","the Maximum Transfer Unit", 
>   OFFSET(mss),AV_OPT_TYPE_INT,{ .i64 = -1 
> }, -1, INT_MAX,   .flags = D|E },

Doesn't tell me about what unit the option is in. Same for most of the other
options. Bytes? Seconds? Great British Pounds?

> +static int srt_neterrno(void)

I know technically (void) is needed in C, but we don't do this in the codebase, 
AFAIK.
> +int err = 0;
> +err = srt_getlasterror(NULL);

err = 0 is a dead store.

> +if (err == SRT_EASYNCRCV)
> +return AVERROR(EAGAIN);
> +return err;

Does err even mean anything meaningful now? The return code
for this is mixing AVERROR codes and SRT_ error codes...

> +static int srt_network_wait_fd(int fd, int write)

Does this kind of fd passing work on Winows properly? I seem to
recall passing int fds around directly in Windows could be iffy.
Ignore this if I'm mis-remembering. 

> +{
> +SRTSOCKET ready[2];
> +int len = 2;
> +int ret = -1;
> +int eid = 0;

Dead stores.

> +static void log_net_error(void *ctx, int level, const char* prefix)
> +{
> +av_log(ctx, level, "%s %s\n", prefix, srt_getlasterror_str());
> +}

Pointless wrapper.

> +static struct addrinfo *srt_resolve_host(URLContext *h,
> + const char *hostname, int port,
> +  

Re: [FFmpeg-devel] [PATCH] doc/filters: note min. resolution for pixscope

[Derek Buitenhuis]

On 10/10/2017 11:42 AM, Gyan Doshi wrote:
> The pixscope filter requires its input to have dimensions of at least 
> 640x480.

Looks fine.

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

Re: [FFmpeg-devel] warning: unknown warning option '-Wno-bool-operation'; did you mean '-Wno-bool-conversion'? [-Wunknown-warning-option]

[Derek Buitenhuis]

On 10/10/2017 8:26 PM, Helmut K. C. Tessarek wrote:
>> And then reapplied in a different form:
>> 
> Yep, and there they are again:
> warning: unknown warning option '-Wno-bool-operation'; did you mean
> '-Wno-bool-conversion'? [-Wunknown-warning-option]
> 

This is weird. it's using check_disable_warning which should only add it if it 
exists...

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

Re: [FFmpeg-devel] [PATCH] avfilter/vf_tile: increase max tile size

[Derek Buitenhuis]

On 10/10/2017 8:22 PM, Paul B Mahol wrote:
> Useful when producing tiles of 1xN or Nx1 size, 1024 is too small then.
> 
> Signed-off-by: Paul B Mahol 
> ---
>  libavfilter/vf_tile.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Is there a reason this is hardcoded and not a user option?

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

Re: [FFmpeg-devel] [PATCH] avfilter/vf_tile: increase max tile size

[Derek Buitenhuis]

On 10/10/2017 8:39 PM, Paul B Mahol wrote:
> One can not set max/min for AV_OPT_TYPE_IMAGE_SIZE.

So use a different type, or implement the option differently?

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

Re: [FFmpeg-devel] [PATCH] avfilter/vf_tile: increase max tile size

[Derek Buitenhuis]

On 10/10/2017 8:59 PM, Paul B Mahol wrote:
> Not possible, there is option used from start.

It's possible to deprecate and add a new one.

> Beside its small change. Why do you care at all about lavfi?

Cool it.

I get it's a simple change, and I'm not blocking it.

However, hardcoding limits is pretty silly/hacky. I suspect there
will be another email N months form now to bump it again, so why
not just fix it properly instead of hacking it?

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

Re: [FFmpeg-devel] [PATCH] Fix for paletteuse to support transparency

[Derek Buitenhuis]

On 10/10/2017 10:41 PM, Bjorn Roche wrote:
> Partial fix for https://trac.ffmpeg.org/ticket/4443
> (Animated GIFs are incorrectly optimized, so they still won’t be output
> correctly).
> ---
>  libavfilter/vf_paletteuse.c | 210 
> 
>  1 file changed, 132 insertions(+), 78 deletions(-)

Commit message should contain info on what changed and why, not just a bug URL.

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

Re: [FFmpeg-devel] [PATCH] Fix for paletteuse to support transparency

[Derek Buitenhuis]

On 10/11/2017 3:04 PM, Bjorn Roche wrote:
> Can you explain where I went wrong? Is "Fix for paletteuse to support
> transparency" incorrect or not sufficient? Do you require that I repeat the
> message in the body as well? Or perhaps I invoked my git commands
> incorrectly?

That tells me what the end result is, but now what you changed, and not
why you changed it (reasoning, context, etc.).

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

Re: [FFmpeg-devel] [PATCH] avfilter/vf_tile: increase max tile size

[Derek Buitenhuis]

On 10/11/2017 8:46 AM, Paul B Mahol wrote:
> Check could be removed, and add check for overflows.

Seems slightly more reasonable to me, I think...

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

Re: [FFmpeg-devel] [PATCH] avformat/srt: add Haivision Open SRT protocol

[Derek Buitenhuis]

On 10/11/2017 4:57 AM, nablet developer wrote:
> you're absolutely right - this patch is based on udp.c (actually I 
> initially copied udp.c and replaced bsd sockets to corresponding SRT 
> calls, with some additional modifications).

Is it possible to deduplicate / share code with udp.c? It seems really iffy to
duplicate a large swath of code. For example, a bunch of my review comments may
also correspond to udp.c (in fact they may originate from udp.c).

> thanks for the review, I am going to correct all errors, as well as run 
> recommended tools.

[...]

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

Re: [FFmpeg-devel] [PATCH] avformat/srt: add Haivision Open SRT protocol

[Derek Buitenhuis]

On 10/11/2017 12:08 PM, Nablet Developer wrote:
> sure, what do you suggest? use "libopensrt" or just "libsrt"? the second 
> one wasn't recommended, because it introduces confusion with subtitle 
> format library.

Probably the former, but I have no real strong opinion on changing it.

> it's documented very briefly (few words for each parameter) in 
> srt/srt.h. I'll remove reference to private header.

That's a shame, but I suppose this isn't an FFmpeg problem.

>>> +static int srt_neterrno(void)
>> I know technically (void) is needed in C, but we don't do this in the 
>> codebase, AFAIK.
> this throws compiler warning (GCC 5.4.0):
> warning: function declaration isn’t a prototype [-Wstrict-prototypes]

Hmm, wasn't aware we used that warning by default. Fine to leave it as-is, if 
so.

> it will fail anyway, because it will return res (which is NULL) below. 
> make it more explicit?

No, that's fine. Thanks for the explanation.

> sorry, I didn't get this one - there is only one such ternary check in 
> code (for localaddr).

I'm saying all user-provided options should be checked in a single place, 
during init,
rather than all over the place in the code, where they are used.

> this one is intentional - getaddrinfo may return multiple addresses 
> (e.g. IPv4 and IPv6), and if we can't open the first, we still can have 
> a chance with second and so one. if none is opened, then it will fail below.

OK. Maybe a av_log debug or comment. Thanks for the explanation.
> this one is actually used and set into url_get_file_handle of 
> URLProtocol. or shall I omit it?

Sorry, I missed that; apologies. It's fine as-is.

>> [...]
> sorry, what do you mean by "[...]"?

Literally just means 'co comment, continue reading below.'

>>> +static int srt_set_options_pre(URLContext *h, int srt_fd)
>>> +{
>>> +SRTContext *s = h->priv_data;
>> You should not be defining functions using the srt_ namespaces, considering
>> this is the namespace of libsrt, which you are using!

^ Making sure this isn't forgot ^

>> If I'm reading this right, you're setting what's supposed to be a user 
>> option?
>>
>> That seems... bad. Same for otehr instances.
> my understanding is that protocols allows to set options as part of URL, 
> e.g. "udp://127.0.0.1:?reuse_socket=1" shall set reuse_socket option 
> as well. is it acceptable

So user options are overridden by URL params...?

>>> +} else {
>>> +/* FIXME: using the monotonic clock would be better,
>>> +   but it does not exist on all supported platforms. */
>> Can we provide an internal or external API to enable its use when available?
> if it's question to me - I don't know. what would you recommend to do in 
> this patch?

It's kind of outside the scope of this patch, but I think an internal API
would be useful. I don't think implementing such an API is a requirement
for this patch, just a nice-to-have.

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

Re: [FFmpeg-devel] [PATCH] avutil/crc: always use precalculated CRC tables for known polynomials

[Derek Buitenhuis]

On 10/22/2017 2:03 PM, James Almer wrote:
> This prevents data races in av_crc_get_table()
> 
> Signed-off-by: James Almer 
> ---
>  libavutil/Makefile |1 +
>  libavutil/crc.c|  295 +-
>  libavutil/crc_tables.c | 1030 
> 
>  libavutil/crc_tables.h |   33 ++
>  4 files changed, 1066 insertions(+), 293 deletions(-)
>  create mode 100644 libavutil/crc_tables.c
>  create mode 100644 libavutil/crc_tables.h

Can this be generated at init, or lazily, using ff_thread_once instead of
hardcoding huge tables?

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

Re: [FFmpeg-devel] [PATCH] avutil/crc: always use precalculated CRC tables for known polynomials

[Derek Buitenhuis]

On 10/22/2017 2:11 PM, James Almer wrote:
> It was suggested, but nobody gave it a try (Or they did but found it
> wasn't as simple as first thought?).

[...]

> Thread sanitizer complains about this in every other run, and the tables
> are at most 1k each, so this is not a bad solution and can be replaced
> by ff_thread_once in the future.

And say that N times and you end up with Nk of tables ;).

That said, I'm not NAKing the patch, just suggesting an alternative.

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

Re: [FFmpeg-devel] [PATCH] avutil/crc: always use precalculated CRC tables for known polynomials

[Derek Buitenhuis]

On 10/23/2017 3:56 AM, Michael Niedermayer wrote:
> the initialization should be thread safe as it never writes a different
> value in the same spot
> This would avoid the need to alwas hardcode the tables

Still no reason to *not* put it under ff_thread_once, though,
to minimize work done.

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

Re: [FFmpeg-devel] [PATCH] libavcodec/h263dec.c: Duplicate the last decoded frame when xvid marks the packet as skipped.

[Derek Buitenhuis]

On 10/25/2017 2:42 AM, Thierry Foucu wrote:
> Changed the return value when no VOD were encoded in Mpeg4 bistream.
> And if we do have already a decoded frames and we are not in xvid_packed
> mode, output the existing decoded frame instead of nothing.
> ---
>  libavcodec/h263dec.c   | 9 -
>  libavcodec/mpeg4videodec.c | 2 +-
>  libavcodec/mpegutils.h | 1 +
>  3 files changed, 10 insertions(+), 2 deletions(-)

Would this considerably slow down pathological files with a lot of
NVOPs purposely inserted? For example, it use to be A Thing to use
NVOPs to hack in "variable framerate" Xvid-in-avi encodes, by for
example, setting the fps to 120 for a file with 24 and 30 fps sections,
and padding the rest with NVOPs.

Do we care about such files? (Probably not...)

I can provide some samples, probably, if needed.

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

Re: [FFmpeg-devel] [PATCH] libavcodec/h263dec.c: Duplicate the last decoded frame when xvid marks the packet as skipped.

[Derek Buitenhuis]

This time to the list properly... woops.

On 10/25/2017 7:38 PM, Thierry Foucu wrote:
> I tried using one of those files.
> Without the patch, the decoder will return only 29 frames
> With this patch, the decoder will return 1947 frames.
> The time of decoding the file were the same (with or without)

Makes sense (because of reference counting).

> I think where it may slow down, it's in case we transcode the file and we
> are not doing constant frame rate, then the encoder will have to encode
> 1947 frames instead of 24 frames.
> But for decoding it the same.

Yeah, that's what I thought might happen. However, thinking about it
a bit more, these files are old and rare enough that we probably shouldn't
care about such an edge case, and the decoder outputting duplicate frames
is probably easier for the API user.

So, no objection from me.

- Derek

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

Re: [FFmpeg-devel] [PATCH] avcodec/wmv2dec: Check end of bitstream in parse_mb_skip() and ff_wmv2_decode_mb()

[Derek Buitenhuis]

On 10/26/2017 11:47 AM, Michael Niedermayer wrote:
> +if (get_bits_left(&s->gb) < 0) {
> +return AVERROR_INVALIDDATA;
> +}

Is this possible? I don't see where get_bits.h is include
in this (probably deep in some other header), so can't see
if it's using the unchecked reader.

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

Re: [FFmpeg-devel] [PATCH]lswr/swresample: Mention actually supported formats when erroring out

[Derek Buitenhuis]

On 10/26/2017 4:03 PM, Carl Eugen Hoyos wrote:
> Hi!
> 
> Attached patch is supposed to fix ticket #6779.
> 
> Please comment, Carl Eugen

Is the 'p' suffix on each needed, since swr only supports 'planar' audio?

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

Re: [FFmpeg-devel] [PATCH]lswr/swresample: Mention actually supported formats when erroring out

[Derek Buitenhuis]

On 10/26/2017 5:13 PM, Carl Eugen Hoyos wrote:
> Not sure I understand:
> Do you mean that the filter option should not require "p" but add
> it always?
> 
> Or do you mean it is obvious for users that only planar
> formats are supported?

If it lines up with the option names, then yes, leave the 'p'.

From the API users' perspective, it's redundant, though, IMO.

I have no strong feelings on it, just wanted to ask.

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

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec.h: remove doxy from the old bsf API functions

[Derek Buitenhuis]

On 10/29/2017 3:49 PM, James Almer wrote:
> Make it clear that these are deprecated and the new API should be
> used instead.
> 
> As a side effect, this reduces differences with libav.
> 
> Signed-off-by: James Almer 
> ---
> With this, existing users will get an extra incentive to migrate, and
> potential new users will not start using the old one by mistake.

I think it's a mistake to not at least leave some sort of "use $thing instead"
in the doxy for each, instead of just one.

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

Re: [FFmpeg-devel] [PATCH] avcodec/avcodec.h: remove doxy from the old bsf API functions

[Derek Buitenhuis]

On 10/29/2017 7:20 PM, James Almer wrote:
> Make it clear that these are deprecated and the new API should be
> used instead.
> 
> As a side effect, this slightly reduces differences with libav.
> 
> Signed-off-by: James Almer 
> ---
>  libavcodec/avcodec.h | 70 
> +++-
>  1 file changed, 14 insertions(+), 56 deletions(-)

OK.

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

Re: [FFmpeg-devel] [PATCH] Fix quadratic memory use in ff_h2645_extract_rbsp() when multiple NALUs exist in packet.

[Derek Buitenhuis]

On 10/31/2017 2:25 AM, Michael Niedermayer wrote:
> (though as said, this fix is not ideal or complete, I would very much
>  prefer if this would be fixed by using a single buffer or any other
>  solution that avoids the speedloss.)

Using a single buffer would be marginally faster, but it does not solve
the underlying problem, which is that the NAL "cache" (nals_allocated)
seems to be cumulative, and the size of each buffer in it seems to be
the largest observed size of a NAL in that position.

Consider I could craft a stream that contains, in order:

Has 1999 tiny NALs, followed by 1 10MiB NAL, in packet 1.
Has 1998 tiny NALs, followed by 1 10MiB NAL, in packet 2.
.
.
.
Has 500 tiny NALs, followed by 1 10MiB NAL, in packet 1500.
.
.
.
And so forth.

The result would be that we have 2000 10MiB buffers allocated
in the NAL memory "pool" (nals_allocated == 2000), which will
persist until the decode is deinitialized.

Am I missing something here?

P.S. I see Kieran mailed the same thing as I wrote this.

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

Re: [FFmpeg-devel] [PATCH] Fix quadratic memory use in ff_h2645_extract_rbsp() when multiple NALUs exist in packet.

[Derek Buitenhuis]

On 11/1/2017 1:36 AM, Michael Niedermayer wrote:
> The idea would be that there would only be one uint8_t buffer and the
> 2000 entries from te pool would point into that.
> So as a larger NAL shifts through the 2000 the pointers would get
> distributed differently but the size would not grow
> Any variable size buffer the H2645NAL needs would be such a "shared"
> buffer

This fixes the type of case I mentioned, but it doesn't fix something, for
example, on a never-ending stream (e.g. a TV channel), if you hit a bunch
of huge NALs in a packet, the single buffer will be resizes to fit them all,
and so instead of a momentary memory usage spike, it lasts forever (or until
you restart the stream / reinit the decoder).

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

Re: [FFmpeg-devel] [PATCH]lavc/alac: Avoid allocating huge memory blocks for malicious alac input.

[Derek Buitenhuis]

On 11/1/2017 2:25 PM, Carl Eugen Hoyos wrote:
> It appears to me that the alac decoder can be used for DoS, the attached patch
> limits the maximum frame size to eight times the default value.
> (Higher values brake our encoder here.)

Since the official ALAC encoder/decoder are open ource nowadays, I took a look
a its source, and it doesn't seem to set any such limit in the encoder or 
decoder.

So, isn't it possible this arbitrary hardcoded limit breaks valid files?

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

Re: [FFmpeg-devel] [PATCH] lavf/mov: Add support for edit list parsing.

[Derek Buitenhuis]

> In YouTube we have long been receiving MOV files from users, which have 
> non-trivial edit lists (Those edit lists which are not just used to offset 
> video start from audio start) and multiple edit lists. Recently the uploads 
> of such files has increased with the introduction of apps that allow video 
> editing and upload like Vine etc. mov.c in ffmpeg does not have edit list 
> parsing support i.e. the support for deciding what video/audio packets should 
> be output with what timestamps, based on edit lists. For this reason, we had 
> built a basic support for edit list parsing in our version of ffmpeg, which 
> fixes the AVIndexEntries built by mov_build_index by looking at edit lists, 
> and introduces new DISCARD flags in AVPacket and AVFrame to signal to the 
> decoder to discard certain packets.
> 
> For a while our edit list support was broken, as in it didn't properly work 
> for multiple edit lists and it had problems with edit-lists ending on 
> B-frames. But we've fixed most of these issues in recent times, and we think 
> that now it is in a good enough condition so that it can be submitted to 
> HEAD. This will not only help the vast userbase of ffmepg, but will also help 
> us with staying up-to-date with ffmpeg and also by adding the power of ffmpeg 
> developer community to our MOV support. So here's a go at it.
> What is supported:
>  - multiple edit lists
>  - edit lists ending on B-frames
>  - zero segment duration edit lists
> 
> What is not supported:
>  - Changing the rate of playing with edit lists. We basically ignore 
> MediaRate field of edit.

Sorry for being late to the party, but this recently broke some code I work on.
I actually re-subscribed to the mailing list just so I could reply to this.

I see two problems with this implementation, one of which breaks things,
and one of which is a code-breaking bug, and one of which is a design problem.

The design one fist: This approach is fundamentally wrong. Edit lists are
meant to be applied at presentation level, not packet level. The current
implementation will cause problems in a number of cases:

* Audio packets. Especially audio packets with a large number of samples.
  It's extremely likely that edits will not fall on packet boundaries, and
  depending on the number of samples per packet, audio sync issues can and
  will occur, even with smaller packets of e.g. 1024 samples, if there are
  a large number of entries in the edit list. Gradual loss of sync will
  occur.
* Edit list entries that are out of order, or repeat. This one is obvious;
  simply dropping packets is not sufficient to create a virtual timeline
  like edit lists can be used for by various NLEs. I need to look up
  if these are actually allowed in the ISOBMFF of QT specs, but I know
  Matroska allows it in its virtual timeline feature.
* Returning timestamps that differ from the codec timestamps. I very
  much disagree with this approach. It's the responsibility of the
  presentation/render/transcode/app/whatever layer to adjust timestamps
  based on edits. I absolutely disagree with libavformat changing
  timestamps from what is coded in the actual file. avformat provides
  demuxers.

Now onto the actual code breaking bug: You are losing packets entirely.

For example, here is part of a diff of pre-edit list ffprobe and post-edit
list ffprobe:

--- old.json2016-10-21 15:35:00.449619260 +0100
+++ new.json2016-10-21 15:35:09.392976730 +0100
@@ -3,28 +3,15 @@
 {
 "codec_type": "audio",
 "stream_index": 1,
-"pts": -2048,
-"pts_time": "-0.046440",
-"dts": -2048,
-"dts_time": "-0.046440",
-"duration": 1024,
-"duration_time": "0.023220",
-"size": "480",
-"pos": "958620",
-"flags": "K"
-},
-{
-"codec_type": "audio",
-"stream_index": 1,
 "pts": -1024,
 "pts_time": "-0.023220",
 "dts": -1024,
 "dts_time": "-0.023220",
 "duration": 1024,
 "duration_time": "0.023220",
-"size": "457",
+"size": "480",
 "pos": "959077",
-"flags": "K"
+"flags": "KD"
 },

As you can see, the packet, sized 457, is entirely missing. This was generated 
via
'ffprobe -show_packets -of json -select_streams 1 sample.mp4'. I can provide the
sample privately to you, if you wish. I did also confirm that the packet as not
added elswwhere:

$ jq '.packets | length' new.json
57050
$ jq '.packets | length' old.json
57051

The hashes I used were today's git HEAD 
(03ec6b780cfae85b8bf0f32b2eda201063ad061b)
for new.json and 590f025b3dc078a6be58b36c67d87499f62b521c for old.j

Re: [FFmpeg-devel] [PATCH] lavf/mov: Add support for edit list parsing.

[Derek Buitenhuis]

On 10/21/2016 4:32 PM, Derek Buitenhuis wrote:
> * Audio packets. Especially audio packets with a large number of samples.
>   It's extremely likely that edits will not fall on packet boundaries, and
>   depending on the number of samples per packet, audio sync issues can and
>   will occur, even with smaller packets of e.g. 1024 samples, if there are
>   a large number of entries in the edit list. Gradual loss of sync will
>   occur.

Apologies, I do see skip_samples is used, but it's unclear if it is correct
or how to use it.

i.e. skip_samples is documented as:

"Number of samples to skip at the start of the frame decoded from the next 
packet."

It's extremely vague, doesn't really say *when* to discard samples. After
start? After a seek? Every packet? Does skip_samples get updated mid-demux?

I've been told it's after every reset (i.e. call to av_codec_flush), but the 
docs
sure don't seem to say this...

Also a friendly reminder that users may not be using libavcodec, but their own
decoder.

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

Re: [FFmpeg-devel] [PATCH] lavf/mov: Add support for edit list parsing.

[Derek Buitenhuis]

On 10/21/2016 6:47 PM, Sasi Inguva wrote:
> * Audio packets. Especially audio packets with a large number of 
> samples.
>   It's extremely likely that edits will not fall on packet 
> boundaries, and
>   depending on the number of samples per packet, audio sync issues 
> can and
>   will occur, even with smaller packets of e.g. 1024 samples, if 
> there are
>   a large number of entries in the edit list. Gradual loss of sync 
> will
>   occur.
> 
> We handle the case of the edit list start boundary not being exactly aligned 
> with a packet boundary, by setting skip_samples fileld properly. But we don't 
> handle the case yet, where edit list boundary might be not aligned with the 
> packet boundary, in which case yes, one might get a gradual loss of sync. 
> (Which is still better than the huge loss of sync one might get if edit lists 
> are not parsed at all! )

FWIW, the gradual loss of sync is very noticeable on some 'popular' apps
that heavily use edit lists, such as "1 Second Everyday: Video Diary", if
I recall. Also some codecs have rather large packets.

> * Edit list entries that are out of order, or repeat. This one is 
> obvious;
>   simply dropping packets is not sufficient to create a virtual 
> timeline
>   like edit lists can be used for by various NLEs. I need to look up
>   if these are actually allowed in the ISOBMFF of QT specs, but I know
>   Matroska allows it in its virtual timeline feature.
> 
> The currrent edit list code supports edits specifying repeated segments, 
> overlapping segments etc.  We are not just dropping packets, but we are 
> rebuilding the whole index. If there are two edits repeating a segmeng (a 
> section of AVPackets lets call it ) , then those section of AVPackets get 
> added twice in the AVIndex, and the decoder would decode the section twice 
> (though I haven't tested such video )  . 
> 
> In fact this is the case where doing it after decode might not work, because 
> something needs to go back and feed those packets again to the decoder.

I see. I had discounted that without looking, I admit, because the idea
of doing that, from a design standpoint, is just... gross, to me.

I definitely don't agree with this approach for a *demuxer*... I guess
this goes back to the conflation of presentation and demuxing layers.

This also requires knowledge of every single codec that can go into
ISOBMFF and what kind of decoding dependencies it has...
 
This whole hack seems very very complex to API users, and in general,
just so that edit lists can be handled on a layer different than what
they're meant to be handled on. It may be OK for ffmpeg.c because it's
'transparent', but it's pretty awful for API users.

> * Returning timestamps that differ from the codec timestamps. I very
>   much disagree with this approach. It's the responsibility of the
>   presentation/render/transcode/app/whatever layer to adjust 
> timestamps
>   based on edits. I absolutely disagree with libavformat changing
>   timestamps from what is coded in the actual file. avformat provides
>   demuxers.
> 
> We already used to apply a global offset to the timestamps of the video, set 
> skip_samples field by parsing the first edit list entry etc. before the edit 
> list patch 
> http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/mov.c;h=6e80b93271a4f998af6ba1af795d7d7c5d67f5a1;hb=7b3bc365f9923e30a925f8dece4fddd127a54c5d#l2792
>   . Not defending the approach, but just pointing  that what I did is just 
> extending that approach to increased functionality.

"We used to have a different hack" isn't justification for introducing
a new, more breaking, hack, really.

> Can you send that file to me please. Thanks.

I have replied to you privately with the file.

Again, I realize I'm way too late to the thread here to make a real
difference, but I thought I should put it on the record.

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

Re: [FFmpeg-devel] [PATCH] lavf/mov.c: Use the correct timescale when seeking for audio.

[Derek Buitenhuis]

On 10/27/2016 10:48 PM, Sasi Inguva wrote:
> gentle ping.
> 
> Thanks!

A ping from me, too.

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

Re: [FFmpeg-devel] [PATCH] lavf/mov.c: Use the correct timescale when seeking for audio.

[Derek Buitenhuis]

On 10/31/2016 1:48 PM, Rostislav Pehlivanov wrote:
> The tests pass on my machine, pushed after Derek said it looks correct.

Hi Sasi,

I notice there's still one difference after this patch. Could you
explain if this is intended:

-"duration": 3072,
-"duration_time": "0.069660",
+"duration": 1016,
+"duration_time": "0.023039",

This is on the very last packet of the file (the same file I sent you before).

Cheers,
- Derek

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