Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata
On Fri, 27 Jan 2017 08:26:26 +0100 Carl Eugen Hoyos wrote: > 2017-01-27 7:04 GMT+01:00 wm4 : > > On Thu, 26 Jan 2017 18:06:39 +0100 > > Carl Eugen Hoyos wrote: > > > >> 2017-01-26 9:26 GMT+01:00 wm4 : > >> > On Thu, 26 Jan 2017 09:16:00 +0100 > >> > Carl Eugen Hoyos wrote: > >> > > >> >> 2017-01-26 9:07 GMT+01:00 wm4 : > >> >> > >> >> >> >> > Any metadata you export can and will get copied to a new file > >> >> >> >> > when > >> >> >> >> > remuxing, therefor exporting arbitrary info that isn't actual > >> >> >> >> > stream > >> >> >> >> > metadata tags in metadata is problematic - it carries over to > >> >> >> >> > the > >> >> >> >> > destination file, in which it would be entirely meaningless. > >> >> >> >> > >> >> >> >> Sorry, I don't understand: > >> >> >> >> Which application do you mean? > >> >> >> > > >> >> >> > ffmpeg > >> >> >> > >> >> >> Didn't I ping yesterday a patch that avoids this? > >> >> >> And wasn't this the mail you answered? > >> >> > > >> >> > Sorry, I couldn't find it just now. What was the subject line? > >> >> > >> >> My mailer (gmail) shows it as the first mail of this thread, so > >> >> it (hopefully) uses the same subject. > >> > > >> > Sorry, I missed that. But this code is not called for remuxing, > >> > or is it? > >> > >> Afair, this behaviour (copying the vendor on remuxing) is > >> consistent with the Apple documentation. > > > > What if you remux to mkv? > > Afair, copying the vendor tag on remuxing is consistent with > its documentation. Well, the Apple docs don't have anything to do with mkv? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata
2017-01-27 7:04 GMT+01:00 wm4 : > On Thu, 26 Jan 2017 18:06:39 +0100 > Carl Eugen Hoyos wrote: > >> 2017-01-26 9:26 GMT+01:00 wm4 : >> > On Thu, 26 Jan 2017 09:16:00 +0100 >> > Carl Eugen Hoyos wrote: >> > >> >> 2017-01-26 9:07 GMT+01:00 wm4 : >> >> >> >> >> >> > Any metadata you export can and will get copied to a new file when >> >> >> >> > remuxing, therefor exporting arbitrary info that isn't actual >> >> >> >> > stream >> >> >> >> > metadata tags in metadata is problematic - it carries over to the >> >> >> >> > destination file, in which it would be entirely meaningless. >> >> >> >> >> >> >> >> Sorry, I don't understand: >> >> >> >> Which application do you mean? >> >> >> > >> >> >> > ffmpeg >> >> >> >> >> >> Didn't I ping yesterday a patch that avoids this? >> >> >> And wasn't this the mail you answered? >> >> > >> >> > Sorry, I couldn't find it just now. What was the subject line? >> >> >> >> My mailer (gmail) shows it as the first mail of this thread, so >> >> it (hopefully) uses the same subject. >> > >> > Sorry, I missed that. But this code is not called for remuxing, >> > or is it? >> >> Afair, this behaviour (copying the vendor on remuxing) is >> consistent with the Apple documentation. > > What if you remux to mkv? Afair, copying the vendor tag on remuxing is consistent with its documentation. Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata
On Thu, 26 Jan 2017 18:06:39 +0100 Carl Eugen Hoyos wrote: > 2017-01-26 9:26 GMT+01:00 wm4 : > > On Thu, 26 Jan 2017 09:16:00 +0100 > > Carl Eugen Hoyos wrote: > > > >> 2017-01-26 9:07 GMT+01:00 wm4 : > >> > >> >> >> > Any metadata you export can and will get copied to a new file when > >> >> >> > remuxing, therefor exporting arbitrary info that isn't actual > >> >> >> > stream > >> >> >> > metadata tags in metadata is problematic - it carries over to the > >> >> >> > destination file, in which it would be entirely meaningless. > >> >> >> > >> >> >> Sorry, I don't understand: > >> >> >> Which application do you mean? > >> >> > > >> >> > ffmpeg > >> >> > >> >> Didn't I ping yesterday a patch that avoids this? > >> >> And wasn't this the mail you answered? > >> > > >> > Sorry, I couldn't find it just now. What was the subject line? > >> > >> My mailer (gmail) shows it as the first mail of this thread, so > >> it (hopefully) uses the same subject. > > > > Sorry, I missed that. But this code is not called for remuxing, > > or is it? > > Afair, this behaviour (copying the vendor on remuxing) is > consistent with the Apple documentation. > What if you remux to mkv? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist
On Fri, 27 Jan 2017 03:03:03 +0100 Michael Niedermayer wrote: > On Thu, Jan 26, 2017 at 02:29:21PM +0100, Paul Arzelier wrote: > > Alright, attached is the last version (I hope!) > > > > Paul > > > > > > Le 26/01/2017 à 13:43, wm4 a écrit : > > >On Thu, 26 Jan 2017 13:32:08 +0100 > > >Paul Arzelier wrote: > > > > > >> From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001 > > >>From: Polochon-street > > >>Date: Thu, 26 Jan 2017 13:25:22 +0100 > > >>Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis > > >> comments) > > >>Originally-by: Ben Boeckel > > >>--- > > >> libavformat/utils.c | 17 - > > >> 1 file changed, 16 insertions(+), 1 deletion(-) > > >> > > >Patch looks generally great to me now. > > > > > >>diff --git a/libavformat/utils.c b/libavformat/utils.c > > >>index d5dfca7dec..ce24244135 100644 > > >>--- a/libavformat/utils.c > > >>+++ b/libavformat/utils.c > > >>@@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const > > >>char *filename, > > >> AVFormatContext *s = *ps; > > >> int i, ret = 0; > > >> AVDictionary *tmp = NULL; > > >>+AVDictionary *id3_meta = NULL; > > >> ID3v2ExtraMeta *id3v2_extra_meta = NULL; > > >> if (!s && !(s = avformat_alloc_context())) > > >>@@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const > > >>char *filename, > > >> /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */ > > >> if (s->pb) > > >>-ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0); > > >>+ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, > > >>&id3v2_extra_meta); > > >> if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header) > > >> if ((ret = s->iformat->read_header(s)) < 0) > > >> goto fail; > > >>+if (!s->metadata) { > > >>+av_dict_copy(&s->metadata, id3_meta, AV_DICT_DONT_OVERWRITE); > > >>+av_dict_free(&id3_meta); > > >Strictly speaking, this line is redundant, but it doesn't harm either. > > >If you feel like it you could remove it, but it's not necessary. > > > > > >>+} > > >>+else if (id3_meta) { > > >If you make another patch, you could merge the } with the next line too > > >(I'm not sure, but I think that's preferred coding-style wise). > > > > > >>+int level = AV_LOG_WARNING; > > >>+if (s->error_recognition & AV_EF_COMPLIANT) > > >>+level = AV_LOG_ERROR; > > >>+av_log(s, level, "Discarding ID3 tag because more suitable tags > > >>were found.\n"); > > >>+if (s->error_recognition & AV_EF_EXPLODE) > > >>+return AVERROR_INVALIDDATA; > > >>+} > > >>+ > > >> if (id3v2_extra_meta) { > > >> if (!strcmp(s->iformat->name, "mp3") || > > >> !strcmp(s->iformat->name, "aac") || > > >> !strcmp(s->iformat->name, "tta")) { > > >>@@ -627,6 +641,7 @@ int avformat_open_input(AVFormatContext **ps, const > > >>char *filename, > > >> fail: > > >> ff_id3v2_free_extra_meta(&id3v2_extra_meta); > > >> av_dict_free(&tmp); > > >>+ av_dict_free(&id3_meta); > > >> if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO)) > > >> avio_closep(&s->pb); > > >> avformat_free_context(s); > > >___ > > >ffmpeg-devel mailing list > > >ffmpeg-devel@ffmpeg.org > > >http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > -- > > Paul ARZELIER > > Élève ingénieur à l'École Centrale de Lille > > 2014-2017 > > > > > utils.c | 17 - > > 1 file changed, 16 insertions(+), 1 deletion(-) > > 477d4f87058a098464e2c1dbfe7e7ff806d2c85f > > 0001-Ignore-ID3-tags-if-other-tags-are-present-e.g.-vorbi.patch > > From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001 > > From: Polochon-street > > Date: Thu, 26 Jan 2017 13:25:22 +0100 > > Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis > > comments) > > Originally-by: Ben Boeckel > > --- > > libavformat/utils.c | 17 - > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/libavformat/utils.c b/libavformat/utils.c > > index d5dfca7dec..ce24244135 100644 > > --- a/libavformat/utils.c > > +++ b/libavformat/utils.c > > @@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const > > char *filename, > > AVFormatContext *s = *ps; > > int i, ret = 0; > > AVDictionary *tmp = NULL; > > +AVDictionary *id3_meta = NULL; > > ID3v2ExtraMeta *id3v2_extra_meta = NULL; > > > > if (!s && !(s = avformat_alloc_context())) > > @@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const > > char *filename, > > > > /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */ > > if (s->pb) > > -ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0); > > +ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, > > &id3v2_
Re: [FFmpeg-devel] [PATCH] HTTP: optimize forward seek performance
Attached is a git format-patch file 0001-HTTP-improve-performance-by-reducing-forward-seeks.patch Description: Binary data > On Jan 26, 2017, at 6:52 PM, Michael Niedermayer wrote: > > On Thu, Jan 26, 2017 at 10:44:33AM -0600, Joel Cunningham wrote: >> Michael, >> >> Thanks for the review and testing! New patch included, see comments below >> >>> >>> this segfaults with many fuzzed files >>> backtrace looks like this: >>> >>> #0 0x7fffb368 in ?? () >>> #1 0x005f9280 in avio_seek (s=0x7fffb278, offset=31, whence=0) >>> at libavformat/aviobuf.c:259 >>> >> >> I found I wasn’t setting s->short_seek_get to NULL in ffio_init_context() >> and was relying on the zeroing aspect of av_mallocz() in >> avio_alloc_context(). From a grep of the source, it looks like >> fifo_init_context() can be called from other functions without having zero’d >> the AVIOContext first. >> >> Is the fuzz test exercising the AVIOContext in this manner? I’d also like >> to reproduce this test if there are instructions >> diff --git a/libavformat/avio.c b/libavformat/avio.c index 3606eb0..ecf6801 100644 --- a/libavformat/avio.c +++ b/libavformat/avio.c @@ -645,6 +645,13 @@ int ffurl_get_multi_file_handle(URLContext *h, int **handles, int *numhandles) return h->prot->url_get_multi_file_handle(h, handles, numhandles); } +int ffurl_get_short_seek(URLContext *h) +{ +if (!h->prot->url_get_short_seek) >>> +return -1; >>> >>> should be some AVERROR code >> >> Fixed >> +return h->prot->url_get_short_seek(h); +} + int ffurl_shutdown(URLContext *h, int flags) { if (!h->prot->url_shutdown) diff --git a/libavformat/avio.h b/libavformat/avio.h index b1ce1d1..0480981 100644 --- a/libavformat/avio.h +++ b/libavformat/avio.h @@ -287,6 +287,12 @@ typedef struct AVIOContext { int short_seek_threshold; /** + * A callback that is used instead of short_seek_threshold. + * This is current internal only, do not use from outside. + */ +int (*short_seek_get)(void *opaque); + +/** * ',' separated list of allowed protocols. */ const char *protocol_whitelist; >>> >>> thats not ok to add this way, the docs say this: >>> /** >>> * Bytestream IO Context. >>> * New fields can be added to the end with minor version bumps. >>> * Removal, reordering and changes to existing fields require a major >>> * version bump. >>> * sizeof(AVIOContext) must not be used outside libav*. >>> * >>> * @note None of the function pointers in AVIOContext should be called >>> * directly, they should only be set by the client application >>> * when implementing custom I/O. Normally these are set to the >>> * function pointers specified in avio_alloc_context() >>> */ >>> >> >> I moved short_seek_get to the end of AVIOContext. I’m not sure if the minor >> version bump referenced in the comment requires any in-code changes. Is >> this an acceptable magnitude of change for this feature? >> >>> [...] --- a/libavformat/tcp.c +++ b/libavformat/tcp.c @@ -265,6 +265,26 @@ static int tcp_get_file_handle(URLContext *h) return s->fd; } +static int tcp_get_window_size(URLContext *h) +{ +TCPContext *s = h->priv_data; +int avail; +int avail_len = sizeof(avail); + >>> +#if HAVE_WINSOCK2_H >>> >>> this formating is IIRC not allowed in C the # must be in the first >>> column >>> >>> +/* SO_RCVBUF with winsock only reports the actual TCP window size when +auto-tuning has been disabled via setting SO_RCVBUF */ +if (s->recv_buffer_size < 0) { +return AVERROR(ENOSYS); +} +#endif + +if (getsockopt(s->fd, SOL_SOCKET, SO_RCVBUF, &avail, &avail_len)) { + return ff_neterrno(); +} >>> >>> the indention is inconsisntent >> >> Both formatting issues have been corrected >> >> From bac0f351afd14c56c56376d20557b7330bcea23e Mon Sep 17 00:00:00 2001 >> From: Joel Cunningham >> Date: Fri, 13 Jan 2017 10:52:25 -0600 >> Subject: [PATCH] HTTP: improve performance by reducing forward seeks > > > please attach a proper git format-patch or use git send-email > > applying this is not as smooth and easy as it should be > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > If you fake or manipulate statistics in a paper in physics you will never > get a job again. > If you fake or manipulate statistics in a paper in medicin you will get > a job for life at the pharma industry. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mail
Re: [FFmpeg-devel] GSoC 2017
On Thu, Jan 26, 2017 at 03:34:24PM -0200, Pedro Arthur wrote: > Hi all, > I can be a backup mentor if needed. we need more (backup) mentors, so yes, i think its needed also if you want to be mentor for a swscale related project i would support that idea [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Opposition brings concord. Out of discord comes the fairest harmony. -- Heraclitus signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 9/9] boadec: prevent overflow during block alignment calculation
On Thu, Jan 26, 2017 at 02:14:09AM +0100, Andreas Cadhalpun wrote: > Signed-off-by: Andreas Cadhalpun > --- > libavformat/boadec.c | 14 +- > 1 file changed, 13 insertions(+), 1 deletion(-) LGTM thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist
On Thu, Jan 26, 2017 at 02:29:21PM +0100, Paul Arzelier wrote: > Alright, attached is the last version (I hope!) > > Paul > > > Le 26/01/2017 à 13:43, wm4 a écrit : > >On Thu, 26 Jan 2017 13:32:08 +0100 > >Paul Arzelier wrote: > > > >> From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001 > >>From: Polochon-street > >>Date: Thu, 26 Jan 2017 13:25:22 +0100 > >>Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis > >> comments) > >>Originally-by: Ben Boeckel > >>--- > >> libavformat/utils.c | 17 - > >> 1 file changed, 16 insertions(+), 1 deletion(-) > >> > >Patch looks generally great to me now. > > > >>diff --git a/libavformat/utils.c b/libavformat/utils.c > >>index d5dfca7dec..ce24244135 100644 > >>--- a/libavformat/utils.c > >>+++ b/libavformat/utils.c > >>@@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const > >>char *filename, > >> AVFormatContext *s = *ps; > >> int i, ret = 0; > >> AVDictionary *tmp = NULL; > >>+AVDictionary *id3_meta = NULL; > >> ID3v2ExtraMeta *id3v2_extra_meta = NULL; > >> if (!s && !(s = avformat_alloc_context())) > >>@@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const > >>char *filename, > >> /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */ > >> if (s->pb) > >>-ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0); > >>+ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, > >>&id3v2_extra_meta); > >> if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header) > >> if ((ret = s->iformat->read_header(s)) < 0) > >> goto fail; > >>+if (!s->metadata) { > >>+av_dict_copy(&s->metadata, id3_meta, AV_DICT_DONT_OVERWRITE); > >>+av_dict_free(&id3_meta); > >Strictly speaking, this line is redundant, but it doesn't harm either. > >If you feel like it you could remove it, but it's not necessary. > > > >>+} > >>+else if (id3_meta) { > >If you make another patch, you could merge the } with the next line too > >(I'm not sure, but I think that's preferred coding-style wise). > > > >>+int level = AV_LOG_WARNING; > >>+if (s->error_recognition & AV_EF_COMPLIANT) > >>+level = AV_LOG_ERROR; > >>+av_log(s, level, "Discarding ID3 tag because more suitable tags > >>were found.\n"); > >>+if (s->error_recognition & AV_EF_EXPLODE) > >>+return AVERROR_INVALIDDATA; > >>+} > >>+ > >> if (id3v2_extra_meta) { > >> if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, > >> "aac") || > >> !strcmp(s->iformat->name, "tta")) { > >>@@ -627,6 +641,7 @@ int avformat_open_input(AVFormatContext **ps, const > >>char *filename, > >> fail: > >> ff_id3v2_free_extra_meta(&id3v2_extra_meta); > >> av_dict_free(&tmp); > >>+ av_dict_free(&id3_meta); > >> if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO)) > >> avio_closep(&s->pb); > >> avformat_free_context(s); > >___ > >ffmpeg-devel mailing list > >ffmpeg-devel@ffmpeg.org > >http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > -- > Paul ARZELIER > Élève ingénieur à l'École Centrale de Lille > 2014-2017 > > utils.c | 17 - > 1 file changed, 16 insertions(+), 1 deletion(-) > 477d4f87058a098464e2c1dbfe7e7ff806d2c85f > 0001-Ignore-ID3-tags-if-other-tags-are-present-e.g.-vorbi.patch > From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001 > From: Polochon-street > Date: Thu, 26 Jan 2017 13:25:22 +0100 > Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis > comments) > Originally-by: Ben Boeckel > --- > libavformat/utils.c | 17 - > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index d5dfca7dec..ce24244135 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char > *filename, > AVFormatContext *s = *ps; > int i, ret = 0; > AVDictionary *tmp = NULL; > +AVDictionary *id3_meta = NULL; > ID3v2ExtraMeta *id3v2_extra_meta = NULL; > > if (!s && !(s = avformat_alloc_context())) > @@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const > char *filename, > > /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */ > if (s->pb) > -ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0); > +ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, > &id3v2_extra_meta); > > if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header) > if ((ret = s->iformat->read_header(s)) < 0) > goto fail; > > +if (!s->metadata) { > +s->metadata = id3_meta; > +id3_meta = NULL; > +} else if (id3_meta) { > +int level = AV_LOG_WARNING;
Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation
On Thu, 26 Jan 2017, Michael Niedermayer wrote: On Thu, Jan 26, 2017 at 03:52:04AM +0100, Marton Balint wrote: On Thu, 26 Jan 2017, Michael Niedermayer wrote: On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote: On 26.01.2017 02:29, Ronald S. Bultje wrote: On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < andreas.cadhal...@googlemail.com> wrote: Signed-off-by: Andreas Cadhalpun --- libavformat/nistspheredec.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c index 782d1dfbfb..3386497682 100644 --- a/libavformat/nistspheredec.c +++ b/libavformat/nistspheredec.c @@ -21,6 +21,7 @@ #include "libavutil/avstring.h" #include "libavutil/intreadwrite.h" +#include "libavcodec/internal.h" #include "avformat.h" #include "internal.h" #include "pcm.h" @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) return 0; } else if (!memcmp(buffer, "channel_count", 13)) { sscanf(buffer, "%*s %*s %"SCNd32, &st->codecpar->channels); +if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { +av_log(s, AV_LOG_ERROR, "Too many channels %d > %d\n", + st->codecpar->channels, FF_SANE_NB_CHANNELS); +return AVERROR(ENOSYS); +} I've said this before, but again - please don't add useless log messages. I disagree that these log messages are useless and I'm not the only one [1]. +1 Log messages make debuging the code easier, as a developer i like to know why something fails having a hard failure but no clear and easy findable error message is bad I tend to agree with Ronald here, log messages which are practically impossible to trigger with real-world files have little benefit, also I don't think it is ffmpeg's job to thoroughly explain every different kind of error. would you want a log message be removed if the people working on the code want the error message to be there ? You seem to just say that you see little benefit in it. Yeah, because as far as I understand the reason behind this, it is only there to let the fuzzer people know why a fuzzed file fails. If we don't draw the line somewhere, ffmpeg will be an analyzer and not a decoder. I see 3 problems (wm4 explicitly named them, but I also had them in mind) - Little benefit, yet - Makes the code less clean, more cluttered - Increases binary size The ideas I proposed (use macros, use common / factorized checks for common validatons and errors) might be a good compromise IMHO. Fuzzing thereforce can be done with "debug" builds. Anyway, I am not blocking the patch, just expressing what I would prefer in the long run. Regards, Marton ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] GSoC 2017
Hi, I am interested in joining one of the projects. Regards Mayank On Thu, Jan 26, 2017 at 11:04 PM, Pedro Arthur wrote: > Hi all, > I can be a backup mentor if needed. > > 2017-01-25 10:15 GMT-02:00 Thilo Borgmann : > > > Am 25.01.17 um 06:14 schrieb Umair Khan: > > > On Wed, Jan 25, 2017 at 7:45 AM, Michael Niedermayer > > > wrote: > > >> > > >> On Mon, Jan 23, 2017 at 03:39:09PM +0100, Michael Niedermayer wrote: > > >>> Hi all > > >>> > > >>> GSoC 2017 mentor org registration has opened a few days ago > > >>> > > >>> Our current ideas page lists just a single mentor and single > sugestion > > >>> http://trac.ffmpeg.org/wiki/SponsoringPrograms/GSoC/2017 > > >>> > > >>> About the Suggested project > > >>> It is a hard design task which few students would be > > >>> capable to do. I dont know if someone already knows a student who is > > >>> qualified for this and who would be available in FFmpeg GSoC but if > not > > >>> I think finding a qualified student will be hard or require some luck > > >>> and success with a less than qualified student unlikely. > > >>> At least thats what my gut feeling says based on past years > > >>> > > >>> About the number of ideas > > >>> one is not enough, and i think its a waste of time if we apply with > > >>> such short list, i doubt we would be accepted with one idea and one > > >>> mentor > > >>> > > >>> About the number of mentors > > >>> one is not enough > > >> > > >> ok so our page now contains > > >> 4 project ideas and 3 mentors > > >> big thanks to the people volunteering and adding them > > >> > > >> Can we get a few more ? > > >> Ideal would be numbers similar to previous years > > >> We wont have a student for each idea we list most likely. > > >> If we list just 4 we might have just 1 or 2 students, having a > > >> wider range of ideas to select from would increase the number of > > >> contributions and number of potential new developers joining. > > > > > > One idea would be to finish rest of the ALS codec tasks. > > > What's remaining is :- > > > > > 1. Implement RLS LMS mode in the decoder. > > > > See 3. > > > > > > > 2. Get the encoder merged into the main repository. > > > > This was part of last years (yours) project... I admit that this has not > > yet happened is due to my lack of time for working on it with you (which > > will most likely change soon :) ) > > However I would not like to have this iterated but being finished by the > > start of the next GSoC round (which basically means by yesterday...). > > > > > > > 3. Encoder featureset still lacks support for floating point sample > > > data encoding. > > > > 1. & 3. could be part of a "complete feature set for ALS"-project. Both > > are too little for separated projects and too much for a qualification > > task. But I like the idea of a comprehensive feature set project. You > came > > up with this idea first so if you're up to mentor it, go ahead and add it > > to the wiki. Otherwise I'll do it during the weekend. > > > > -Thilo > > > > ___ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] lavc/mjpegdec: hint next marker position in ff_mjpeg_find_marker
On Thu, Jan 26, 2017 at 05:47:51PM +0100, Matthieu Bouron wrote: > Speeds up next marker search when a SOS marker is found. > > ff_mjpeg_find_marker goes from 54% to 30% under perf record ffmpeg -i > sample_2992x4000.jpg > --- > libavcodec/mjpegdec.c | 31 +-- > libavcodec/mjpegdec.h | 2 +- > libavcodec/mxpegdec.c | 5 +++-- > 3 files changed, 29 insertions(+), 9 deletions(-) this breaks decoding multiple jpeg files for example tickets//856/nint.jpg [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship naturally arises out of democracy, and the most aggravated form of tyranny and slavery out of the most extreme liberty. -- Plato signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] doc/muxers: remove confusing example for segment muxer option clocktime_wrap_duration
Detecting a leap second depends on a lot of things, segment time, segment offset, system leap second implementation, the removed part is a huge simplification which can be misleading, so it is best to remove it. Signed-off-by: Marton Balint --- doc/muxers.texi | 3 --- 1 file changed, 3 deletions(-) diff --git a/doc/muxers.texi b/doc/muxers.texi index 26a8f2d..2438ab2 100644 --- a/doc/muxers.texi +++ b/doc/muxers.texi @@ -1408,9 +1408,6 @@ within the specified duration after the segmenting clock time. This way you can make the segmenter more resilient to backward local time jumps, such as leap seconds or transition to standard time from daylight savings time. -Assuming that the delay between the packets of your source is less than 0.5 -second you can detect a leap second by specifying 0.5 as the duration. - Default is the maximum possible duration which means starting a new segment regardless of the elapsed time since the last clock time. -- 2.10.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] HTTP: optimize forward seek performance
On Thu, Jan 26, 2017 at 10:44:33AM -0600, Joel Cunningham wrote: > Michael, > > Thanks for the review and testing! New patch included, see comments below > > > > > this segfaults with many fuzzed files > > backtrace looks like this: > > > > #0 0x7fffb368 in ?? () > > #1 0x005f9280 in avio_seek (s=0x7fffb278, offset=31, whence=0) > > at libavformat/aviobuf.c:259 > > > > I found I wasn’t setting s->short_seek_get to NULL in ffio_init_context() and > was relying on the zeroing aspect of av_mallocz() in avio_alloc_context(). > From a grep of the source, it looks like fifo_init_context() can be called > from other functions without having zero’d the AVIOContext first. > > Is the fuzz test exercising the AVIOContext in this manner? I’d also like to > reproduce this test if there are instructions > > >> > >> diff --git a/libavformat/avio.c b/libavformat/avio.c > >> index 3606eb0..ecf6801 100644 > >> --- a/libavformat/avio.c > >> +++ b/libavformat/avio.c > >> @@ -645,6 +645,13 @@ int ffurl_get_multi_file_handle(URLContext *h, int > >> **handles, int *numhandles) > >> return h->prot->url_get_multi_file_handle(h, handles, numhandles); > >> } > >> > >> +int ffurl_get_short_seek(URLContext *h) > >> +{ > >> +if (!h->prot->url_get_short_seek) > > > >> +return -1; > > > > should be some AVERROR code > > Fixed > > >> +return h->prot->url_get_short_seek(h); > >> +} > >> + > >> int ffurl_shutdown(URLContext *h, int flags) > >> { > >> if (!h->prot->url_shutdown) > >> diff --git a/libavformat/avio.h b/libavformat/avio.h > >> index b1ce1d1..0480981 100644 > >> --- a/libavformat/avio.h > >> +++ b/libavformat/avio.h > >> @@ -287,6 +287,12 @@ typedef struct AVIOContext { > >> int short_seek_threshold; > >> > >> /** > >> + * A callback that is used instead of short_seek_threshold. > >> + * This is current internal only, do not use from outside. > >> + */ > >> +int (*short_seek_get)(void *opaque); > >> + > >> +/** > >> * ',' separated list of allowed protocols. > >> */ > >> const char *protocol_whitelist; > > > > thats not ok to add this way, the docs say this: > > /** > > * Bytestream IO Context. > > * New fields can be added to the end with minor version bumps. > > * Removal, reordering and changes to existing fields require a major > > * version bump. > > * sizeof(AVIOContext) must not be used outside libav*. > > * > > * @note None of the function pointers in AVIOContext should be called > > * directly, they should only be set by the client application > > * when implementing custom I/O. Normally these are set to the > > * function pointers specified in avio_alloc_context() > > */ > > > > I moved short_seek_get to the end of AVIOContext. I’m not sure if the minor > version bump referenced in the comment requires any in-code changes. Is this > an acceptable magnitude of change for this feature? > > > [...] > >> --- a/libavformat/tcp.c > >> +++ b/libavformat/tcp.c > >> @@ -265,6 +265,26 @@ static int tcp_get_file_handle(URLContext *h) > >> return s->fd; > >> } > >> > >> +static int tcp_get_window_size(URLContext *h) > >> +{ > >> +TCPContext *s = h->priv_data; > >> +int avail; > >> +int avail_len = sizeof(avail); > >> + > > > >> +#if HAVE_WINSOCK2_H > > > > this formating is IIRC not allowed in C the # must be in the first > > column > > > > > >> +/* SO_RCVBUF with winsock only reports the actual TCP window size when > >> +auto-tuning has been disabled via setting SO_RCVBUF */ > >> +if (s->recv_buffer_size < 0) { > >> +return AVERROR(ENOSYS); > >> +} > >> +#endif > >> + > >> +if (getsockopt(s->fd, SOL_SOCKET, SO_RCVBUF, &avail, &avail_len)) { > >> + return ff_neterrno(); > >> +} > > > > the indention is inconsisntent > > Both formatting issues have been corrected > > From bac0f351afd14c56c56376d20557b7330bcea23e Mon Sep 17 00:00:00 2001 > From: Joel Cunningham > Date: Fri, 13 Jan 2017 10:52:25 -0600 > Subject: [PATCH] HTTP: improve performance by reducing forward seeks please attach a proper git format-patch or use git send-email applying this is not as smooth and easy as it should be [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you fake or manipulate statistics in a paper in physics you will never get a job again. If you fake or manipulate statistics in a paper in medicin you will get a job for life at the pharma industry. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat: parse iTunes gapless information
Signed-off-by: Christopher Snowhill --- libavformat/mp3dec.c | 77 +++- 1 file changed, 76 insertions(+), 1 deletion(-) diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index 099ca57..b895e37 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -295,6 +295,66 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base) } } +static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration) +{ +uint32_t v; +AVDictionaryEntry *de; +MP3DecContext *mp3 = s->priv_data; +size_t length; +uint32_t zero, start_pad, end_pad; +uint64_t last_eight_frames_offset; +int64_t temp_duration, file_size; +int i; + +if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0))) +return; + +length = strlen(de->value); + +/* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type + * There are four fields we need in the first six, the rest are currently zero padding */ +if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11)) +return; + +file_size = avio_size(s->pb); +if (file_size < 0) +file_size = 0; + +if (sscanf(de->value, "%"PRIx32" %"PRIx32" %"PRIx32" %"PRIx64" %"PRIx32" %"PRIx64, &zero, &start_pad, &end_pad, &temp_duration, &zero, &last_eight_frames_offset) < 6 || +temp_duration < 0 || +start_pad > (576 * 2 * 32) || +end_pad > (576 * 2 * 64) || +(file_size && (last_eight_frames_offset >= (file_size - base - vbrtag_size { +*duration = 0; +return; +} + +*duration = temp_duration; + +mp3->start_pad = start_pad; +mp3->end_pad = end_pad; +if (end_pad >= 528 + 1) +mp3->end_pad = end_pad - (528 + 1); +st->start_skip_samples = mp3->start_pad + 528 + 1; +av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad); +if (!s->pb->seekable) +return; + +*size = (unsigned int) last_eight_frames_offset; +if (avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, SEEK_SET) < 0) +return; + +for (i = 0; i < 8; i++) { +v = avio_rb32(s->pb); +if (ff_mpa_check_header(v) < 0) +return; +if (avpriv_mpegaudio_decode_header(c, v) != 0) +break; +*size += c->frame_size; +avio_skip(s->pb, c->frame_size - 4); +} +} + /** * Try to find Xing/Info/VBRI tags and compute duration from info therein */ @@ -303,8 +363,10 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) uint32_t v, spf; MPADecodeHeader c; int vbrtag_size = 0; +unsigned int size = 0; MP3DecContext *mp3 = s->priv_data; int ret; +uint64_t duration = 0; ffio_init_checksum(s->pb, ff_crcA001_update, 0); @@ -327,16 +389,29 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, AVStream *st, int64_t base) mp3_parse_vbri_tag(s, st, base); if (!mp3->frames && !mp3->header_filesize) +vbrtag_size = 0; + +mp3_parse_itunes_tag(s, st, &c, base, vbrtag_size, &size, &duration); + +if (!mp3->frames && !size && !duration) return -1; /* Skip the vbr tag frame */ avio_seek(s->pb, base + vbrtag_size, SEEK_SET); -if (mp3->frames) +if (duration) +st->duration = av_rescale_q(duration, (AVRational){1, c.sample_rate}, st->time_base); +else if (mp3->frames) st->duration = av_rescale_q(mp3->frames, (AVRational){spf, c.sample_rate}, st->time_base); if (mp3->header_filesize && mp3->frames && !mp3->is_cbr) st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * c.sample_rate, mp3->frames * (int64_t)spf); +if (size) { +if (duration) +st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, duration); +else if (mp3->frames) +st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, mp3->frames * (int64_t)spf); +} return 0; } -- 2.10.1.windows.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [FFmpeg-devel, PATCHv5] avformat: parse iTunes gapless information
Let's try this again. I can't believe I missed all those things you mentioned. It's almost as if I didn't actually proofread my patch before sending it. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/9] electronicarts: prevent overflow during block alignment calculation
On Thu, Jan 26, 2017 at 02:11:31AM +0100, Andreas Cadhalpun wrote: > Signed-off-by: Andreas Cadhalpun > --- > libavformat/electronicarts.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) LGTM thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB During times of universal deceit, telling the truth becomes a revolutionary act. -- George Orwell signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] lavf/matroskaenc.c: Free dyn bufs in mkv_free. Fixes memory leaks when muxing fails.
Signed-off-by: Sasi Inguva --- libavformat/matroskaenc.c | 17 + 1 file changed, 17 insertions(+) diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index f731b678b9..88f6c647b9 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -393,6 +393,23 @@ static void put_xiph_size(AVIOContext *pb, int size) * Free the members allocated in the mux context. */ static void mkv_free(MatroskaMuxContext *mkv) { +uint8_t* buf; +if (mkv->dyn_bc) { +avio_close_dyn_buf(mkv->dyn_bc, &buf); +av_free(buf); +} +if (mkv->info_bc) { +avio_close_dyn_buf(mkv->info_bc, &buf); +av_free(buf); +} +if (mkv->tracks_bc) { +avio_close_dyn_buf(mkv->tracks_bc, &buf); +av_free(buf); +} +if (mkv->tags_bc) { +avio_close_dyn_buf(mkv->tags_bc, &buf); +av_free(buf); +} if (mkv->main_seekhead) { av_freep(&mkv->main_seekhead->entries); av_freep(&mkv->main_seekhead); -- 2.11.0.483.g087da7b7c-goog ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/Makefile: fix compilation of testprogs when networking is disabled
On Wed, Jan 25, 2017 at 03:05:55PM +0100, Tobias Rapp wrote: > Signed-off-by: Tobias Rapp > --- > libavformat/Makefile | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) probably ok thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] tcp: set socket buffer sizes before listen/connect/accept
On Wed, Jan 25, 2017 at 09:06:36AM -0600, Joel Cunningham wrote: > Ping! Anyone have a chance to look at this issue/patch? > > Thanks, patch applied thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In a rich man's house there is no place to spit but his face. -- Diogenes of Sinope signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] fate: add SCC test
On Wed, Jan 25, 2017 at 10:30:06PM +0100, Paul B Mahol wrote: > Signed-off-by: Paul B Mahol > --- > tests/fate/subtitles.mak | 3 + > tests/ref/fate/sub-scc | 519 > +++ > 2 files changed, 522 insertions(+) > create mode 100644 tests/ref/fate/sub-scc Tested on linux32/64 x86, mingw32/64, mips/arm qemu linux [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Republics decline into democracies and democracies degenerate into despotisms. -- Aristotle signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] GSoC 2017
On Mon, Jan 23, 2017 at 03:39:09PM +0100, Michael Niedermayer wrote: > Hi all > > GSoC 2017 mentor org registration has opened a few days ago Btw, Outreachy also has a new round, if someone wants to register FFmpeg (after confirming that we are willing to fund 1 slot) February 9 is the deadline for that [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Does the universe only have a finite lifespan? No, its going to go on forever, its just that you wont like living in it. -- Hiranya Peiri signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] GSoC 2017
Hi all, I can be a backup mentor if needed. 2017-01-25 10:15 GMT-02:00 Thilo Borgmann : > Am 25.01.17 um 06:14 schrieb Umair Khan: > > On Wed, Jan 25, 2017 at 7:45 AM, Michael Niedermayer > wrote: > >> > >> On Mon, Jan 23, 2017 at 03:39:09PM +0100, Michael Niedermayer wrote: > >>> Hi all > >>> > >>> GSoC 2017 mentor org registration has opened a few days ago > >>> > >>> Our current ideas page lists just a single mentor and single sugestion > >>> http://trac.ffmpeg.org/wiki/SponsoringPrograms/GSoC/2017 > >>> > >>> About the Suggested project > >>> It is a hard design task which few students would be > >>> capable to do. I dont know if someone already knows a student who is > >>> qualified for this and who would be available in FFmpeg GSoC but if not > >>> I think finding a qualified student will be hard or require some luck > >>> and success with a less than qualified student unlikely. > >>> At least thats what my gut feeling says based on past years > >>> > >>> About the number of ideas > >>> one is not enough, and i think its a waste of time if we apply with > >>> such short list, i doubt we would be accepted with one idea and one > >>> mentor > >>> > >>> About the number of mentors > >>> one is not enough > >> > >> ok so our page now contains > >> 4 project ideas and 3 mentors > >> big thanks to the people volunteering and adding them > >> > >> Can we get a few more ? > >> Ideal would be numbers similar to previous years > >> We wont have a student for each idea we list most likely. > >> If we list just 4 we might have just 1 or 2 students, having a > >> wider range of ideas to select from would increase the number of > >> contributions and number of potential new developers joining. > > > > One idea would be to finish rest of the ALS codec tasks. > > What's remaining is :- > > > 1. Implement RLS LMS mode in the decoder. > > See 3. > > > > 2. Get the encoder merged into the main repository. > > This was part of last years (yours) project... I admit that this has not > yet happened is due to my lack of time for working on it with you (which > will most likely change soon :) ) > However I would not like to have this iterated but being finished by the > start of the next GSoC round (which basically means by yesterday...). > > > > 3. Encoder featureset still lacks support for floating point sample > > data encoding. > > 1. & 3. could be part of a "complete feature set for ALS"-project. Both > are too little for separated projects and too much for a qualification > task. But I like the idea of a comprehensive feature set project. You came > up with this idea first so if you're up to mentor it, go ahead and add it > to the wiki. Otherwise I'll do it during the weekend. > > -Thilo > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata
2017-01-26 9:26 GMT+01:00 wm4 : > On Thu, 26 Jan 2017 09:16:00 +0100 > Carl Eugen Hoyos wrote: > >> 2017-01-26 9:07 GMT+01:00 wm4 : >> >> >> >> > Any metadata you export can and will get copied to a new file when >> >> >> > remuxing, therefor exporting arbitrary info that isn't actual stream >> >> >> > metadata tags in metadata is problematic - it carries over to the >> >> >> > destination file, in which it would be entirely meaningless. >> >> >> >> >> >> Sorry, I don't understand: >> >> >> Which application do you mean? >> >> > >> >> > ffmpeg >> >> >> >> Didn't I ping yesterday a patch that avoids this? >> >> And wasn't this the mail you answered? >> > >> > Sorry, I couldn't find it just now. What was the subject line? >> >> My mailer (gmail) shows it as the first mail of this thread, so >> it (hopefully) uses the same subject. > > Sorry, I missed that. But this code is not called for remuxing, > or is it? Afair, this behaviour (copying the vendor on remuxing) is consistent with the Apple documentation. Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] lavc/mjpegdec: hint next marker position in ff_mjpeg_find_marker
Speeds up next marker search when a SOS marker is found. ff_mjpeg_find_marker goes from 54% to 30% under perf record ffmpeg -i sample_2992x4000.jpg --- libavcodec/mjpegdec.c | 31 +-- libavcodec/mjpegdec.h | 2 +- libavcodec/mxpegdec.c | 5 +++-- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c index 7d17e3dfcb..29f40cb4e6 100644 --- a/libavcodec/mjpegdec.c +++ b/libavcodec/mjpegdec.c @@ -1921,13 +1921,23 @@ static int mjpeg_decode_com(MJpegDecodeContext *s) /* return the 8 bit start code value and update the search state. Return -1 if no start code found */ -static int find_marker(const uint8_t **pbuf_ptr, const uint8_t *buf_end) +static int find_marker(const uint8_t **pbuf_ptr, const uint8_t *buf_hint, const uint8_t *buf_end) { const uint8_t *buf_ptr; unsigned int v, v2; int val; int skipped = 0; +if (buf_hint && (buf_end - buf_hint > 1)) { +v = *buf_hint++; +v2 = *buf_hint; +if ((v == 0xff) && (v2 >= 0xc0) && (v2 <= 0xfe) && buf_hint < buf_end) { +buf_ptr = buf_hint++; +val = v2; +goto found; +} +} + buf_ptr = *pbuf_ptr; while (buf_end - buf_ptr > 1) { v = *buf_ptr++; @@ -1947,12 +1957,15 @@ found: } int ff_mjpeg_find_marker(MJpegDecodeContext *s, - const uint8_t **buf_ptr, const uint8_t *buf_end, + const uint8_t **buf_ptr, + const uint8_t **buf_hint, + const uint8_t *buf_end, const uint8_t **unescaped_buf_ptr, int *unescaped_buf_size) { int start_code; -start_code = find_marker(buf_ptr, buf_end); +start_code = find_marker(buf_ptr, *buf_hint, buf_end); +*buf_hint = NULL; av_fast_padded_malloc(&s->buffer, &s->buffer_size, buf_end - *buf_ptr); if (!s->buffer) @@ -1963,6 +1976,7 @@ int ff_mjpeg_find_marker(MJpegDecodeContext *s, const uint8_t *src = *buf_ptr; const uint8_t *ptr = src; uint8_t *dst = s->buffer; +const uint8_t *next_marker = NULL; #define copy_data_segment(skip) do { \ ptrdiff_t length = (ptr - src) - (skip); \ @@ -1999,8 +2013,10 @@ int ff_mjpeg_find_marker(MJpegDecodeContext *s, if (x < 0xd0 || x > 0xd7) { copy_data_segment(1); -if (x) +if (x) { +next_marker = ptr - 2; break; +} } } } @@ -2009,6 +2025,8 @@ int ff_mjpeg_find_marker(MJpegDecodeContext *s, } #undef copy_data_segment +if (next_marker) +*buf_hint = next_marker; *unescaped_buf_ptr = s->buffer; *unescaped_buf_size = dst - s->buffer; memset(s->buffer + *unescaped_buf_size, 0, @@ -2073,7 +2091,7 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, const uint8_t *buf = avpkt->data; int buf_size = avpkt->size; MJpegDecodeContext *s = avctx->priv_data; -const uint8_t *buf_end, *buf_ptr; +const uint8_t *buf_end, *buf_hint, *buf_ptr; const uint8_t *unescaped_buf_ptr; int hshift, vshift; int unescaped_buf_size; @@ -2087,10 +2105,11 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, s->adobe_transform = -1; buf_ptr = buf; +buf_hint = NULL; buf_end = buf + buf_size; while (buf_ptr < buf_end) { /* find start next marker */ -start_code = ff_mjpeg_find_marker(s, &buf_ptr, buf_end, +start_code = ff_mjpeg_find_marker(s, &buf_ptr, &buf_hint, buf_end, &unescaped_buf_ptr, &unescaped_buf_size); /* EOF */ diff --git a/libavcodec/mjpegdec.h b/libavcodec/mjpegdec.h index fb811294a1..c9d289187b 100644 --- a/libavcodec/mjpegdec.h +++ b/libavcodec/mjpegdec.h @@ -144,7 +144,7 @@ int ff_mjpeg_decode_sos(MJpegDecodeContext *s, const uint8_t *mb_bitmask,int mb_bitmask_size, const AVFrame *reference); int ff_mjpeg_find_marker(MJpegDecodeContext *s, - const uint8_t **buf_ptr, const uint8_t *buf_end, + const uint8_t **buf_ptr, const uint8_t **buf_hint, const uint8_t *buf_end, const uint8_t **unescaped_buf_ptr, int *unescaped_buf_size); #endif /* AVCODEC_MJPEGDEC_H */ diff --git a/libavcodec/mxpegdec.c b/libavcodec/mxpegdec.c index 2e3ebe6e70..28a479a3b5 100644 --- a/libavcodec/mxpegdec.c +++ b/libavcodec/mxpegdec.c @@ -189,18 +189,19 @@ static int mxpeg_decode_frame(AVCodecContext *avctx, int buf_size = avpkt->size; MXpegDecodeCon
Re: [FFmpeg-devel] [PATCH] HTTP: optimize forward seek performance
Michael, Thanks for the review and testing! New patch included, see comments below > > this segfaults with many fuzzed files > backtrace looks like this: > > #0 0x7fffb368 in ?? () > #1 0x005f9280 in avio_seek (s=0x7fffb278, offset=31, whence=0) > at libavformat/aviobuf.c:259 > I found I wasn’t setting s->short_seek_get to NULL in ffio_init_context() and was relying on the zeroing aspect of av_mallocz() in avio_alloc_context(). From a grep of the source, it looks like fifo_init_context() can be called from other functions without having zero’d the AVIOContext first. Is the fuzz test exercising the AVIOContext in this manner? I’d also like to reproduce this test if there are instructions >> >> diff --git a/libavformat/avio.c b/libavformat/avio.c >> index 3606eb0..ecf6801 100644 >> --- a/libavformat/avio.c >> +++ b/libavformat/avio.c >> @@ -645,6 +645,13 @@ int ffurl_get_multi_file_handle(URLContext *h, int >> **handles, int *numhandles) >> return h->prot->url_get_multi_file_handle(h, handles, numhandles); >> } >> >> +int ffurl_get_short_seek(URLContext *h) >> +{ >> +if (!h->prot->url_get_short_seek) > >> +return -1; > > should be some AVERROR code Fixed >> +return h->prot->url_get_short_seek(h); >> +} >> + >> int ffurl_shutdown(URLContext *h, int flags) >> { >> if (!h->prot->url_shutdown) >> diff --git a/libavformat/avio.h b/libavformat/avio.h >> index b1ce1d1..0480981 100644 >> --- a/libavformat/avio.h >> +++ b/libavformat/avio.h >> @@ -287,6 +287,12 @@ typedef struct AVIOContext { >> int short_seek_threshold; >> >> /** >> + * A callback that is used instead of short_seek_threshold. >> + * This is current internal only, do not use from outside. >> + */ >> +int (*short_seek_get)(void *opaque); >> + >> +/** >> * ',' separated list of allowed protocols. >> */ >> const char *protocol_whitelist; > > thats not ok to add this way, the docs say this: > /** > * Bytestream IO Context. > * New fields can be added to the end with minor version bumps. > * Removal, reordering and changes to existing fields require a major > * version bump. > * sizeof(AVIOContext) must not be used outside libav*. > * > * @note None of the function pointers in AVIOContext should be called > * directly, they should only be set by the client application > * when implementing custom I/O. Normally these are set to the > * function pointers specified in avio_alloc_context() > */ > I moved short_seek_get to the end of AVIOContext. I’m not sure if the minor version bump referenced in the comment requires any in-code changes. Is this an acceptable magnitude of change for this feature? > [...] >> --- a/libavformat/tcp.c >> +++ b/libavformat/tcp.c >> @@ -265,6 +265,26 @@ static int tcp_get_file_handle(URLContext *h) >> return s->fd; >> } >> >> +static int tcp_get_window_size(URLContext *h) >> +{ >> +TCPContext *s = h->priv_data; >> +int avail; >> +int avail_len = sizeof(avail); >> + > >> +#if HAVE_WINSOCK2_H > > this formating is IIRC not allowed in C the # must be in the first > column > > >> +/* SO_RCVBUF with winsock only reports the actual TCP window size when >> +auto-tuning has been disabled via setting SO_RCVBUF */ >> +if (s->recv_buffer_size < 0) { >> +return AVERROR(ENOSYS); >> +} >> +#endif >> + >> +if (getsockopt(s->fd, SOL_SOCKET, SO_RCVBUF, &avail, &avail_len)) { >> + return ff_neterrno(); >> +} > > the indention is inconsisntent Both formatting issues have been corrected From bac0f351afd14c56c56376d20557b7330bcea23e Mon Sep 17 00:00:00 2001 From: Joel Cunningham Date: Fri, 13 Jan 2017 10:52:25 -0600 Subject: [PATCH] HTTP: improve performance by reducing forward seeks This commit optimizes HTTP performance by reducing forward seeks, instead favoring a read-ahead and discard on the current connection (referred to as a short seek) for seeks that are within a TCP window's worth of data. This improves performance because with TCP flow control, a window's worth of data will be in the local socket buffer already or in-flight from the sender once congestion control on the sender is fully utilizing the window. Note: this approach doesn't attempt to differentiate from a newly opened connection which may not be fully utilizing the window due to congestion control vs one that is. The receiver can't get at this information, so we assume worst case; that full window is in use (we did advertise it after all) and that data could be in-flight The previous behavior of closing the connection, then opening a new with a new HTTP range value results in a massive amounts of discarded and re-sent data when large TCP windows are used. This has been observed on MacOS/iOS which starts with an initial window of 256KB and grows up to 1MB depending on the bandwidth-product delay. When seeking within a window's worth of data and we close the connectio
Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation
On Thu, Jan 26, 2017 at 06:43:45AM +0100, wm4 wrote: > On Thu, 26 Jan 2017 03:20:02 +0100 > Michael Niedermayer wrote: > > > On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote: > > > On 26.01.2017 02:29, Ronald S. Bultje wrote: > > > > On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < > > > > andreas.cadhal...@googlemail.com> wrote: > > > > > > > >> Signed-off-by: Andreas Cadhalpun > > > >> --- > > > >> libavformat/nistspheredec.c | 11 +++ > > > >> 1 file changed, 11 insertions(+) > > > >> > > > >> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c > > > >> index 782d1dfbfb..3386497682 100644 > > > >> --- a/libavformat/nistspheredec.c > > > >> +++ b/libavformat/nistspheredec.c > > > >> @@ -21,6 +21,7 @@ > > > >> > > > >> #include "libavutil/avstring.h" > > > >> #include "libavutil/intreadwrite.h" > > > >> +#include "libavcodec/internal.h" > > > >> #include "avformat.h" > > > >> #include "internal.h" > > > >> #include "pcm.h" > > > >> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) > > > >> return 0; > > > >> } else if (!memcmp(buffer, "channel_count", 13)) { > > > >> sscanf(buffer, "%*s %*s %"SCNd32, > > > >> &st->codecpar->channels); > > > >> +if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { > > > >> +av_log(s, AV_LOG_ERROR, "Too many channels %d > %d\n", > > > >> + st->codecpar->channels, FF_SANE_NB_CHANNELS); > > > >> +return AVERROR(ENOSYS); > > > >> +} > > > > > > > > > > > > I've said this before, but again - please don't add useless log > > > > messages. > > > > > > I disagree that these log messages are useless and I'm not the only one > > > [1]. > > > > +1 > > > > Log messages make debuging the code easier, as a developer i like to > > know why something fails having a hard failure but no clear and easy > > findable error message is bad > > > > > > [...] > > -1 > > This kind of things bloat the code with rare corner cases. One point > would be that this increases binary size (why do we even still have the > NULL_IF_CONFIG_SMALL macro? I'm not saying we should use it here, but > the current use of the macro will barely even make a dent into the > number of bytes "wasted" for optional strings, yet we bother). > > Another point is that code becomes unreadable if obscure corner cases > take up most of the code. I think that's w worrisome direction we're > taking here. While it doesnt apply to this patch here but, Error messages in obscure checks that describe the error condition in the source would make at least some checks easier to understand than just a generic "return AVERROR_INVALIDDATA" > > When I debug FFmpeg API use, the thing that annoys me most is that I > can't know where an error happens. I have to trace the origin manually. > Error codes are almost always completely useless. This patchset adds a > lot of NOSYS, which is used 254 times in FFmpeg and which is about 99% > meaningless and resolves to an even worse error message when using > av_err2str(). Adding an av_log to error error point would "work" (at > least you could use grep), but isn't a good solution. I think the idea about something more informative than a int32 error code did come up previously. I certainly would be in favor of having an error value that could be used to pinpoint the error location(s) and or function(s) it passed through, this would be usefull for debguging in general [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is what and why we do it that matters, not just one of them. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist
On Thu, 26 Jan 2017 14:29:21 +0100 Paul Arzelier wrote: > From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001 > From: Polochon-street > Date: Thu, 26 Jan 2017 13:25:22 +0100 > Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis > comments) > Originally-by: Ben Boeckel > --- > libavformat/utils.c | 17 - > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index d5dfca7dec..ce24244135 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char > *filename, > AVFormatContext *s = *ps; > int i, ret = 0; > AVDictionary *tmp = NULL; > +AVDictionary *id3_meta = NULL; > ID3v2ExtraMeta *id3v2_extra_meta = NULL; > > if (!s && !(s = avformat_alloc_context())) > @@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const > char *filename, > > /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */ > if (s->pb) > -ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0); > +ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, > &id3v2_extra_meta); > > if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header) > if ((ret = s->iformat->read_header(s)) < 0) > goto fail; > > +if (!s->metadata) { > +s->metadata = id3_meta; > +id3_meta = NULL; > +} else if (id3_meta) { > +int level = AV_LOG_WARNING; > +if (s->error_recognition & AV_EF_COMPLIANT) > +level = AV_LOG_ERROR; > +av_log(s, level, "Discarding ID3 tag because more suitable tags were > found.\n"); > +av_dict_free(&id3_meta); > +if (s->error_recognition & AV_EF_EXPLODE) > +return AVERROR_INVALIDDATA; > +} > + > if (id3v2_extra_meta) { > if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, > "aac") || > !strcmp(s->iformat->name, "tta")) { > @@ -627,6 +641,7 @@ int avformat_open_input(AVFormatContext **ps, const char > *filename, > fail: > ff_id3v2_free_extra_meta(&id3v2_extra_meta); > av_dict_free(&tmp); > +av_dict_free(&id3_meta); > if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO)) > avio_closep(&s->pb); > avformat_free_context(s); LGTM ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist
Alright, attached is the last version (I hope!) Paul Le 26/01/2017 à 13:43, wm4 a écrit : On Thu, 26 Jan 2017 13:32:08 +0100 Paul Arzelier wrote: From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001 From: Polochon-street Date: Thu, 26 Jan 2017 13:25:22 +0100 Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis comments) Originally-by: Ben Boeckel --- libavformat/utils.c | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) Patch looks generally great to me now. diff --git a/libavformat/utils.c b/libavformat/utils.c index d5dfca7dec..ce24244135 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, AVFormatContext *s = *ps; int i, ret = 0; AVDictionary *tmp = NULL; +AVDictionary *id3_meta = NULL; ID3v2ExtraMeta *id3v2_extra_meta = NULL; if (!s && !(s = avformat_alloc_context())) @@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */ if (s->pb) -ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0); +ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta); if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header) if ((ret = s->iformat->read_header(s)) < 0) goto fail; +if (!s->metadata) { +av_dict_copy(&s->metadata, id3_meta, AV_DICT_DONT_OVERWRITE); +av_dict_free(&id3_meta); Strictly speaking, this line is redundant, but it doesn't harm either. If you feel like it you could remove it, but it's not necessary. +} +else if (id3_meta) { If you make another patch, you could merge the } with the next line too (I'm not sure, but I think that's preferred coding-style wise). +int level = AV_LOG_WARNING; +if (s->error_recognition & AV_EF_COMPLIANT) +level = AV_LOG_ERROR; +av_log(s, level, "Discarding ID3 tag because more suitable tags were found.\n"); +if (s->error_recognition & AV_EF_EXPLODE) +return AVERROR_INVALIDDATA; +} + if (id3v2_extra_meta) { if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, "aac") || !strcmp(s->iformat->name, "tta")) { @@ -627,6 +641,7 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, fail: ff_id3v2_free_extra_meta(&id3v2_extra_meta); av_dict_free(&tmp); + av_dict_free(&id3_meta); if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO)) avio_closep(&s->pb); avformat_free_context(s); ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel -- Paul ARZELIER Élève ingénieur à l'École Centrale de Lille 2014-2017 >From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001 From: Polochon-street Date: Thu, 26 Jan 2017 13:25:22 +0100 Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis comments) Originally-by: Ben Boeckel --- libavformat/utils.c | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index d5dfca7dec..ce24244135 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, AVFormatContext *s = *ps; int i, ret = 0; AVDictionary *tmp = NULL; +AVDictionary *id3_meta = NULL; ID3v2ExtraMeta *id3v2_extra_meta = NULL; if (!s && !(s = avformat_alloc_context())) @@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */ if (s->pb) -ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0); +ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta); if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header) if ((ret = s->iformat->read_header(s)) < 0) goto fail; +if (!s->metadata) { +s->metadata = id3_meta; +id3_meta = NULL; +} else if (id3_meta) { +int level = AV_LOG_WARNING; +if (s->error_recognition & AV_EF_COMPLIANT) +level = AV_LOG_ERROR; +av_log(s, level, "Discarding ID3 tag because more suitable tags were found.\n"); +av_dict_free(&id3_meta); +if (s->error_recognition & AV_EF_EXPLODE) +return AVERROR_INVALIDDATA; +} + if (id3v2_extra_meta) { if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, "aac") || !strcmp(s->iformat->name, "tta")) { @@ -627,6 +641,7 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, fail: ff_id3v2_free_extra_meta(&id3v2_extra_meta);
Re: [FFmpeg-devel] [PATCH 1/3] mpeg12dec: validate color space
Hi, On Wed, Jan 25, 2017 at 8:56 PM, Andreas Cadhalpun < andreas.cadhal...@googlemail.com> wrote: > On 26.01.2017 02:26, Ronald S. Bultje wrote: > > Hi, > > > > On Wed, Jan 25, 2017 at 8:20 PM, Andreas Cadhalpun < > > andreas.cadhal...@googlemail.com> wrote: > > > >> On 07.01.2017 13:44, Ronald S. Bultje wrote: > >>> Doesn't this destroy exporting of newly added colorspaces that have no > >>> explicit mention yet in libavutil? I'm not 100% sure this is a good > >> thing. > >> > >> Yes. That's how it is already done in libavcodec/h264_ps.c, so it just > >> makes handling unknown values consistent. > > > > > > Changing h264_ps.c would also make things consistent. > > No, because also libavcodec/options_table.h limits the colorspace to known > values. That is input only, this is output. > The question is not whether changing A or B towards the other makes the > > combination consistent. The question is which form of consistency is > > better... > > As new values don't get added that often, I don't see a problem with > requiring > to explicitly add them to libavutil before being able to use them. That is because your use case for libavcodec is constrained, you use it only for decoding videos and fuzzing. There are others in this community that do a lot more than that with libavcodec. Look, Andreas, I appreciate the work you're doing, I really do, but you always pick a fight with every review I put out on your code. I don't understand why. My reviews are not difficult to address, they really are not. My reviews are actionable, and if the action is taken, I'm happy and the code can be committed. Why pick a fight? What is the point? Do you think I'm an idiot that has no clue what he's doing and should be shot down because of that? Please appreciate that I do have some clue of what I'm doing, and I am looking out for the health of the project, just like you, but with a slightly different perspective to some things. If I'm wrong, please point it out, I make mistakes also, but in cases like these, it can also help to just drop it and move on. Resolving an issue is not losing, it is winning. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist
On Thu, 26 Jan 2017 13:32:08 +0100 Paul Arzelier wrote: > From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001 > From: Polochon-street > Date: Thu, 26 Jan 2017 13:25:22 +0100 > Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis > comments) > Originally-by: Ben Boeckel > --- > libavformat/utils.c | 17 - > 1 file changed, 16 insertions(+), 1 deletion(-) > Patch looks generally great to me now. > diff --git a/libavformat/utils.c b/libavformat/utils.c > index d5dfca7dec..ce24244135 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char > *filename, > AVFormatContext *s = *ps; > int i, ret = 0; > AVDictionary *tmp = NULL; > +AVDictionary *id3_meta = NULL; > ID3v2ExtraMeta *id3v2_extra_meta = NULL; > > if (!s && !(s = avformat_alloc_context())) > @@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const > char *filename, > > /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */ > if (s->pb) > -ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0); > +ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, > &id3v2_extra_meta); > > if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header) > if ((ret = s->iformat->read_header(s)) < 0) > goto fail; > > +if (!s->metadata) { > +av_dict_copy(&s->metadata, id3_meta, AV_DICT_DONT_OVERWRITE); > +av_dict_free(&id3_meta); Strictly speaking, this line is redundant, but it doesn't harm either. If you feel like it you could remove it, but it's not necessary. > +} > +else if (id3_meta) { If you make another patch, you could merge the } with the next line too (I'm not sure, but I think that's preferred coding-style wise). > +int level = AV_LOG_WARNING; > +if (s->error_recognition & AV_EF_COMPLIANT) > +level = AV_LOG_ERROR; > +av_log(s, level, "Discarding ID3 tag because more suitable tags were > found.\n"); > +if (s->error_recognition & AV_EF_EXPLODE) > +return AVERROR_INVALIDDATA; > +} > + > if (id3v2_extra_meta) { > if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, > "aac") || > !strcmp(s->iformat->name, "tta")) { > @@ -627,6 +641,7 @@ int avformat_open_input(AVFormatContext **ps, const char > *filename, > fail: > ff_id3v2_free_extra_meta(&id3v2_extra_meta); > av_dict_free(&tmp); > + av_dict_free(&id3_meta); > if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO)) > avio_closep(&s->pb); > avformat_free_context(s); ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist
Ah, missed the leaked, sorry. Regarding the explode message, I kept it because of this message http://ffmpeg.org/pipermail/ffmpeg-devel/2015-February/168250.html . Maybe it is not that relevant anymore though, because we're talking about all existing codecs there, and not only FLAC (which seems to be a bit more strict when it comes down to tags). Le 26/01/2017 à 13:10, wm4 a écrit : On Thu, 26 Jan 2017 12:55:15 +0100 Paul Arzelier wrote: From a3dc6068fb06722aacea56365f948afdb8df841f Mon Sep 17 00:00:00 2001 From: Paul Arzelier Date: Thu, 26 Jan 2017 12:51:33 +0100 Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis comments) Originally-by: Ben Boeckel --- libavformat/utils.c | 16 +++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index d5dfca7dec..bbe5c1ff1c 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, AVFormatContext *s = *ps; int i, ret = 0; AVDictionary *tmp = NULL; +AVDictionary *id3_meta = NULL; Looks like this would leak on "goto fail;". ID3v2ExtraMeta *id3v2_extra_meta = NULL; if (!s && !(s = avformat_alloc_context())) @@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */ if (s->pb) -ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0); +ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta); if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header) if ((ret = s->iformat->read_header(s)) < 0) goto fail; +if (!s->metadata) { +av_dict_copy(&s->metadata, id3_meta, AV_DICT_DONT_OVERWRITE); +av_dict_free(&id3_meta); +} +else if (id3_meta) { +int level = AV_LOG_WARNING; +if (s->error_recognition & AV_EF_COMPLIANT) +level = AV_LOG_ERROR; +av_log(s, level, "Spec-compliant flac files do not support ID3 tags.\n"); Seems ok, but this message will be printed for non-flac formats too, which is weird at best. Maybe something along the id3 tag being discarded or so? +if (s->error_recognition & AV_EF_EXPLODE) +return AVERROR_INVALIDDATA; Not sure about this one, but I'd be fine with it personally. +} + if (id3v2_extra_meta) { if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, "aac") || !strcmp(s->iformat->name, "tta")) { ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel -- Paul ARZELIER Élève ingénieur à l'École Centrale de Lille 2014-2017 >From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001 From: Polochon-street Date: Thu, 26 Jan 2017 13:25:22 +0100 Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis comments) Originally-by: Ben Boeckel --- libavformat/utils.c | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index d5dfca7dec..ce24244135 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, AVFormatContext *s = *ps; int i, ret = 0; AVDictionary *tmp = NULL; +AVDictionary *id3_meta = NULL; ID3v2ExtraMeta *id3v2_extra_meta = NULL; if (!s && !(s = avformat_alloc_context())) @@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */ if (s->pb) -ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0); +ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta); if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header) if ((ret = s->iformat->read_header(s)) < 0) goto fail; +if (!s->metadata) { +av_dict_copy(&s->metadata, id3_meta, AV_DICT_DONT_OVERWRITE); +av_dict_free(&id3_meta); +} +else if (id3_meta) { +int level = AV_LOG_WARNING; +if (s->error_recognition & AV_EF_COMPLIANT) +level = AV_LOG_ERROR; +av_log(s, level, "Discarding ID3 tag because more suitable tags were found.\n"); +if (s->error_recognition & AV_EF_EXPLODE) +return AVERROR_INVALIDDATA; +} + if (id3v2_extra_meta) { if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, "aac") || !strcmp(s->iformat->name, "tta")) { @@ -627,6 +641,7 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, fail: ff_id3v2_free_extra_meta(&id3v2_extra_meta); av_dict_free(&tmp); + av_dict_free(&id3_meta); if (s->pb && !(
Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist
On Thu, 26 Jan 2017 12:55:15 +0100 Paul Arzelier wrote: > From a3dc6068fb06722aacea56365f948afdb8df841f Mon Sep 17 00:00:00 2001 > From: Paul Arzelier > Date: Thu, 26 Jan 2017 12:51:33 +0100 > Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis > comments) > Originally-by: Ben Boeckel > --- > libavformat/utils.c | 16 +++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index d5dfca7dec..bbe5c1ff1c 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char > *filename, > AVFormatContext *s = *ps; > int i, ret = 0; > AVDictionary *tmp = NULL; > +AVDictionary *id3_meta = NULL; Looks like this would leak on "goto fail;". > ID3v2ExtraMeta *id3v2_extra_meta = NULL; > > if (!s && !(s = avformat_alloc_context())) > @@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const > char *filename, > > /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */ > if (s->pb) > -ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0); > +ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, > &id3v2_extra_meta); > > if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header) > if ((ret = s->iformat->read_header(s)) < 0) > goto fail; > > +if (!s->metadata) { > +av_dict_copy(&s->metadata, id3_meta, AV_DICT_DONT_OVERWRITE); > +av_dict_free(&id3_meta); > +} > +else if (id3_meta) { > +int level = AV_LOG_WARNING; > +if (s->error_recognition & AV_EF_COMPLIANT) > +level = AV_LOG_ERROR; > +av_log(s, level, "Spec-compliant flac files do not support ID3 > tags.\n"); Seems ok, but this message will be printed for non-flac formats too, which is weird at best. Maybe something along the id3 tag being discarded or so? > +if (s->error_recognition & AV_EF_EXPLODE) > +return AVERROR_INVALIDDATA; Not sure about this one, but I'd be fine with it personally. > +} > + > if (id3v2_extra_meta) { > if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, > "aac") || > !strcmp(s->iformat->name, "tta")) { ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist
You're right, I made a patch for libavformat/utils.c instead. I modified a bit Ben's version and kept only the "spec-compliant flac files do not support ID3 tags" warning, I hope it's okay with you. Paul Le 26/01/2017 à 11:24, wm4 a écrit : On Thu, 26 Jan 2017 11:08:45 +0100 Paul Arzelier wrote: From bb5dc73c42ad2e0bffd7e38fac18c6f01ec05ab3 Mon Sep 17 00:00:00 2001 From: Paul Arzelier Date: Thu, 26 Jan 2017 11:04:44 +0100 Subject: [PATCH] Fixed behavior when id3 tags were found on FLAC files Originally-by: Ben Boeckel --- libavformat/flacdec.c | 30 ++ 1 file changed, 30 insertions(+) diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c index 66baba5922..869936621b 100644 --- a/libavformat/flacdec.c +++ b/libavformat/flacdec.c @@ -48,6 +48,7 @@ static int flac_read_header(AVFormatContext *s) int ret, metadata_last=0, metadata_type, metadata_size, found_streaminfo=0; uint8_t header[4]; uint8_t *buffer=NULL; +int has_id3 = 0; FLACDecContext *flac = s->priv_data; AVStream *st = avformat_new_stream(s, NULL); if (!st) @@ -63,6 +64,22 @@ static int flac_read_header(AVFormatContext *s) return 0; } +if (av_dict_count(s->metadata)) { + /* XXX: Is there a better way to parse this out? ID3 parsing is done + * all the way out in avformat_open_input. */ + has_id3 = 1; +} + +if (has_id3) { + int level = AV_LOG_WARNING; + if (s->error_recognition & AV_EF_COMPLIANT) + level = AV_LOG_ERROR; + av_log(s, level, "Spec-compliant flac files do not support ID3 tags.\n"); + if (s->error_recognition & AV_EF_EXPLODE) + return AVERROR_INVALIDDATA; +} + + /* process metadata blocks */ while (!avio_feof(s->pb) && !metadata_last) { if (avio_read(s->pb, header, 4) != 4) @@ -173,8 +190,16 @@ static int flac_read_header(AVFormatContext *s) } /* process supported blocks other than STREAMINFO */ if (metadata_type == FLAC_METADATA_TYPE_VORBIS_COMMENT) { +AVDictionary *other_meta = NULL; AVDictionaryEntry *chmask; +if (has_id3) { +av_log(s, AV_LOG_VERBOSE, "FLAC found with ID3 and vorbis tags; ignoring ID3 tags also provided by vorbis.\n"); + +other_meta = s->metadata; +s->metadata = NULL; +} + ret = ff_vorbis_comment(s, &s->metadata, buffer, metadata_size, 1); if (ret < 0) { av_log(s, AV_LOG_WARNING, "error parsing VorbisComment metadata\n"); @@ -182,6 +207,11 @@ static int flac_read_header(AVFormatContext *s) s->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED; } +if (other_meta) { +av_dict_copy(&s->metadata, other_meta, AV_DICT_DONT_OVERWRITE); +av_dict_free(&other_meta); +} + /* parse the channels mask if present */ chmask = av_dict_get(s->metadata, "WAVEFORMATEXTENSIBLE_CHANNEL_MASK", NULL, 0); if (chmask) { I feel like there should be a more general solution. ID3v2 tags are read by common code, and such tags are supported by basically all file formats in libavformat due to the way it's implemented. This fixes it for FLAC only. Are we going to duplicate this for every format? I suggest reading id3 tags into a separate dict instead, and then copying it in libavformat/utils.c after read_header if the demuxer hasn't set any tags. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel -- Paul ARZELIER Élève ingénieur à l'École Centrale de Lille 2014-2017 >From a3dc6068fb06722aacea56365f948afdb8df841f Mon Sep 17 00:00:00 2001 From: Paul Arzelier Date: Thu, 26 Jan 2017 12:51:33 +0100 Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis comments) Originally-by: Ben Boeckel --- libavformat/utils.c | 16 +++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index d5dfca7dec..bbe5c1ff1c 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, AVFormatContext *s = *ps; int i, ret = 0; AVDictionary *tmp = NULL; +AVDictionary *id3_meta = NULL; ID3v2ExtraMeta *id3v2_extra_meta = NULL; if (!s && !(s = avformat_alloc_context())) @@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const char *filename, /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */ if (s->pb) -ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0); +ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAUL
Re: [FFmpeg-devel] [PATCH 2/2] avformat/udp: Replace use of pthread_cancel.
> > > Ive reupped the current versions of each seperated patch for comment > ___ > > Hi, could this patch fix issue #5783? (https://trac.ffmpeg.org/ticket/5783) Thanks, MB ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist
On Thu, 26 Jan 2017 11:08:45 +0100 Paul Arzelier wrote: > From bb5dc73c42ad2e0bffd7e38fac18c6f01ec05ab3 Mon Sep 17 00:00:00 2001 > From: Paul Arzelier > Date: Thu, 26 Jan 2017 11:04:44 +0100 > Subject: [PATCH] Fixed behavior when id3 tags were found on FLAC files > Originally-by: Ben Boeckel > --- > libavformat/flacdec.c | 30 ++ > 1 file changed, 30 insertions(+) > > diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c > index 66baba5922..869936621b 100644 > --- a/libavformat/flacdec.c > +++ b/libavformat/flacdec.c > @@ -48,6 +48,7 @@ static int flac_read_header(AVFormatContext *s) > int ret, metadata_last=0, metadata_type, metadata_size, > found_streaminfo=0; > uint8_t header[4]; > uint8_t *buffer=NULL; > +int has_id3 = 0; > FLACDecContext *flac = s->priv_data; > AVStream *st = avformat_new_stream(s, NULL); > if (!st) > @@ -63,6 +64,22 @@ static int flac_read_header(AVFormatContext *s) > return 0; > } > > +if (av_dict_count(s->metadata)) { > + /* XXX: Is there a better way to parse this out? ID3 parsing is done > + * all the way out in avformat_open_input. */ > + has_id3 = 1; > +} > + > +if (has_id3) { > + int level = AV_LOG_WARNING; > + if (s->error_recognition & AV_EF_COMPLIANT) > + level = AV_LOG_ERROR; > + av_log(s, level, "Spec-compliant flac files do not support ID3 > tags.\n"); > + if (s->error_recognition & AV_EF_EXPLODE) > + return AVERROR_INVALIDDATA; > +} > + > + > /* process metadata blocks */ > while (!avio_feof(s->pb) && !metadata_last) { > if (avio_read(s->pb, header, 4) != 4) > @@ -173,8 +190,16 @@ static int flac_read_header(AVFormatContext *s) > } > /* process supported blocks other than STREAMINFO */ > if (metadata_type == FLAC_METADATA_TYPE_VORBIS_COMMENT) { > +AVDictionary *other_meta = NULL; > AVDictionaryEntry *chmask; > > +if (has_id3) { > +av_log(s, AV_LOG_VERBOSE, "FLAC found with ID3 and > vorbis tags; ignoring ID3 tags also provided by vorbis.\n"); > + > +other_meta = s->metadata; > +s->metadata = NULL; > +} > + > ret = ff_vorbis_comment(s, &s->metadata, buffer, > metadata_size, 1); > if (ret < 0) { > av_log(s, AV_LOG_WARNING, "error parsing VorbisComment > metadata\n"); > @@ -182,6 +207,11 @@ static int flac_read_header(AVFormatContext *s) > s->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED; > } > > +if (other_meta) { > +av_dict_copy(&s->metadata, other_meta, > AV_DICT_DONT_OVERWRITE); > +av_dict_free(&other_meta); > +} > + > /* parse the channels mask if present */ > chmask = av_dict_get(s->metadata, > "WAVEFORMATEXTENSIBLE_CHANNEL_MASK", NULL, 0); > if (chmask) { I feel like there should be a more general solution. ID3v2 tags are read by common code, and such tags are supported by basically all file formats in libavformat due to the way it's implemented. This fixes it for FLAC only. Are we going to duplicate this for every format? I suggest reading id3 tags into a separate dict instead, and then copying it in libavformat/utils.c after read_header if the demuxer hasn't set any tags. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist
Oops, I don't know what went wrong, but now it seems to work. I've attached the working patched, sorry! Le 26/01/2017 à 02:12, Michael Niedermayer a écrit : On Wed, Jan 25, 2017 at 08:33:53PM +0100, Paul Arzelier wrote: Hi all, Would it be possible to continue the discussion began there ? http://ffmpeg.org/pipermail/ffmpeg-devel/2015-February/168248.html The current behavior of FFmpeg when it comes to manage FLAC tags with both vorbis tags and ID3 tags is to append both, which leads to, for example, "Artist: Pink Floyd;Pink Floyd" in FFprobe. This is not a desired output: as the XIPH faq says about FLAC (https://xiph.org/flac/faq.html): "FLAC has it's own native tagging system which is identical to that of Vorbis. They are called alternately "FLAC tags" and "Vorbis comments". It is the only tagging system required and guaranteed to be supported by FLAC implementations. Out of convenience, the reference decoder knows how to skip ID3 tags so that they don't interfere with decoding. But you should not expect any tags beside FLAC tags to be supported in applications; some implementations may not even be able to decode a FLAC file with ID3 tags." This issue was also submitted here https://trac.ffmpeg.org/ticket/3799, but still discuted in the above thread. Ben Boeckel made the attached patch (which I edited a bit to fit FFmpeg last version), which fixes the issue. The problem that remained was that many other files could have irrelevant id3v2 tags, not only Vorbis/FLAC files, so another more global patch may be needed to fix that definitely. So, do you think Ben's patch could be merged "as-is", or does it only raise more questions? Regards, Paul the attached patch is corrupted: Applying: Fixed behavior when id3 tags were found on FLAC files error: corrupt patch at line 58 Patch failed at 0001 Fixed behavior when id3 tags were found on FLAC files The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". [...] ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel -- Paul ARZELIER Élève ingénieur à l'École Centrale de Lille 2014-2017 >From bb5dc73c42ad2e0bffd7e38fac18c6f01ec05ab3 Mon Sep 17 00:00:00 2001 From: Paul Arzelier Date: Thu, 26 Jan 2017 11:04:44 +0100 Subject: [PATCH] Fixed behavior when id3 tags were found on FLAC files Originally-by: Ben Boeckel --- libavformat/flacdec.c | 30 ++ 1 file changed, 30 insertions(+) diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c index 66baba5922..869936621b 100644 --- a/libavformat/flacdec.c +++ b/libavformat/flacdec.c @@ -48,6 +48,7 @@ static int flac_read_header(AVFormatContext *s) int ret, metadata_last=0, metadata_type, metadata_size, found_streaminfo=0; uint8_t header[4]; uint8_t *buffer=NULL; +int has_id3 = 0; FLACDecContext *flac = s->priv_data; AVStream *st = avformat_new_stream(s, NULL); if (!st) @@ -63,6 +64,22 @@ static int flac_read_header(AVFormatContext *s) return 0; } +if (av_dict_count(s->metadata)) { + /* XXX: Is there a better way to parse this out? ID3 parsing is done + * all the way out in avformat_open_input. */ + has_id3 = 1; +} + +if (has_id3) { + int level = AV_LOG_WARNING; + if (s->error_recognition & AV_EF_COMPLIANT) + level = AV_LOG_ERROR; + av_log(s, level, "Spec-compliant flac files do not support ID3 tags.\n"); + if (s->error_recognition & AV_EF_EXPLODE) + return AVERROR_INVALIDDATA; +} + + /* process metadata blocks */ while (!avio_feof(s->pb) && !metadata_last) { if (avio_read(s->pb, header, 4) != 4) @@ -173,8 +190,16 @@ static int flac_read_header(AVFormatContext *s) } /* process supported blocks other than STREAMINFO */ if (metadata_type == FLAC_METADATA_TYPE_VORBIS_COMMENT) { +AVDictionary *other_meta = NULL; AVDictionaryEntry *chmask; +if (has_id3) { +av_log(s, AV_LOG_VERBOSE, "FLAC found with ID3 and vorbis tags; ignoring ID3 tags also provided by vorbis.\n"); + +other_meta = s->metadata; +s->metadata = NULL; +} + ret = ff_vorbis_comment(s, &s->metadata, buffer, metadata_size, 1); if (ret < 0) { av_log(s, AV_LOG_WARNING, "error parsing VorbisComment metadata\n"); @@ -182,6 +207,11 @@ static int flac_read_header(AVFormatContext *s) s->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED; } +if (other_meta) { +av_dict_copy(&s-
Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation
On 1/26/17, wm4 wrote: > On Thu, 26 Jan 2017 03:20:02 +0100 > Michael Niedermayer wrote: > >> On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote: >> > On 26.01.2017 02:29, Ronald S. Bultje wrote: >> > > On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun < >> > > andreas.cadhal...@googlemail.com> wrote: >> > > >> > >> Signed-off-by: Andreas Cadhalpun >> > >> --- >> > >> libavformat/nistspheredec.c | 11 +++ >> > >> 1 file changed, 11 insertions(+) >> > >> >> > >> diff --git a/libavformat/nistspheredec.c >> > >> b/libavformat/nistspheredec.c >> > >> index 782d1dfbfb..3386497682 100644 >> > >> --- a/libavformat/nistspheredec.c >> > >> +++ b/libavformat/nistspheredec.c >> > >> @@ -21,6 +21,7 @@ >> > >> >> > >> #include "libavutil/avstring.h" >> > >> #include "libavutil/intreadwrite.h" >> > >> +#include "libavcodec/internal.h" >> > >> #include "avformat.h" >> > >> #include "internal.h" >> > >> #include "pcm.h" >> > >> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s) >> > >> return 0; >> > >> } else if (!memcmp(buffer, "channel_count", 13)) { >> > >> sscanf(buffer, "%*s %*s %"SCNd32, >> > >> &st->codecpar->channels); >> > >> +if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { >> > >> +av_log(s, AV_LOG_ERROR, "Too many channels %d > >> > >> %d\n", >> > >> + st->codecpar->channels, FF_SANE_NB_CHANNELS); >> > >> +return AVERROR(ENOSYS); >> > >> +} >> > > >> > > >> > > I've said this before, but again - please don't add useless log >> > > messages. >> > >> > I disagree that these log messages are useless and I'm not the only one >> > [1]. >> >> +1 >> >> Log messages make debuging the code easier, as a developer i like to >> know why something fails having a hard failure but no clear and easy >> findable error message is bad >> >> >> [...] > > -1 > > This kind of things bloat the code with rare corner cases. One point > would be that this increases binary size (why do we even still have the > NULL_IF_CONFIG_SMALL macro? I'm not saying we should use it here, but > the current use of the macro will barely even make a dent into the > number of bytes "wasted" for optional strings, yet we bother). > > Another point is that code becomes unreadable if obscure corner cases > take up most of the code. I think that's w worrisome direction we're > taking here. > > When I debug FFmpeg API use, the thing that annoys me most is that I > can't know where an error happens. I have to trace the origin manually. > Error codes are almost always completely useless. This patchset adds a > lot of NOSYS, which is used 254 times in FFmpeg and which is about 99% > meaningless and resolves to an even worse error message when using > av_err2str(). Adding an av_log to error error point would "work" (at > least you could use grep), but isn't a good solution. > > In this particular case, I'd question why the code calling this > function (avformat_open_inputavcodec_open) doesn't print an error > message instead, or so. > > But of course not every API user wants such an error message (but still > wants others). I don't want those log messages in there. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 6/9] pvfdec: prevent overflow during block alignment calculation
On 1/26/17, Andreas Cadhalpun wrote: > Signed-off-by: Andreas Cadhalpun > --- > libavformat/pvfdec.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > lgtm ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata
On Thu, 26 Jan 2017 09:16:00 +0100 Carl Eugen Hoyos wrote: > 2017-01-26 9:07 GMT+01:00 wm4 : > > >> >> > Any metadata you export can and will get copied to a new file when > >> >> > remuxing, therefor exporting arbitrary info that isn't actual stream > >> >> > metadata tags in metadata is problematic - it carries over to the > >> >> > destination file, in which it would be entirely meaningless. > >> >> > >> >> Sorry, I don't understand: > >> >> Which application do you mean? > >> > > >> > ffmpeg > >> > >> Didn't I ping yesterday a patch that avoids this? > >> And wasn't this the mail you answered? > > > > Sorry, I couldn't find it just now. What was the subject line? > > My mailer (gmail) shows it as the first mail of this thread, so > it (hopefully) uses the same subject. Sorry, I missed that. But this code is not called for remuxing, or is it? > > I'm not necessarily blocking your patch. > > > There are already plenty of other cases where it breaks this > > way, > > Where did you report this? > I believe you know how difficult it is to fix bugs that do not get > reported. I think I mentioned it multiple times. I do not report ffmpeg bugs because I'm more productive not doing that, sorry. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] libavformat patch that brute-forces aax encryption
On Tue, 24 Jan 2017 19:48:00 -0700 William Shipley wrote: > I made a small modification of libavformat that bruteforces the 4-byte code > used in audible encrypted files. It automatically runs if an aax is passed > (always encrypted) without the code provided. Previously, it would tell the > user the code was needed and exit. > > It takes between 5 and 10 minutes to crack it as currently implemented, > upon which it performs the specified task (conversion, content extraction, > etc) and outputs the decryption key on the console. > > Is there any interest in including this upstream? If it's a code quality > issue, I'm open to suggestions, but if it's felt that this is outside the > scope of the project or legally risky then I understand. > > I didn't do any kind of reverse engineering or anything legally gray as far > as I know, just noticed that it's literally a 32-bit key after the fixed > key is in place (which was already in ffmpeg code). I used a legally > obtained aax from my own audible account to test it, even. > > The key it outputs is the same key you get from tools like > audible-activator. It's basically a user ID for a login. > > I currently have a fork up on github here: > https://github.com/FFmpeg/FFmpeg/compare/master...willrandship:master > I'll generate a patch file if you're interested. I'm fairly sure this is not really appropriate to put into a demuxer. Especially if it means that opening a file can hang for 5 to 10 minutes eating 100% CPU. It should probably be a separate file. (Could even be in FFmpeg's tools/ directory, so I'm not necessarily rejecting it for this project.) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata
2017-01-26 9:07 GMT+01:00 wm4 : >> >> > Any metadata you export can and will get copied to a new file when >> >> > remuxing, therefor exporting arbitrary info that isn't actual stream >> >> > metadata tags in metadata is problematic - it carries over to the >> >> > destination file, in which it would be entirely meaningless. >> >> >> >> Sorry, I don't understand: >> >> Which application do you mean? >> > >> > ffmpeg >> >> Didn't I ping yesterday a patch that avoids this? >> And wasn't this the mail you answered? > > Sorry, I couldn't find it just now. What was the subject line? My mailer (gmail) shows it as the first mail of this thread, so it (hopefully) uses the same subject. > I'm not necessarily blocking your patch. > There are already plenty of other cases where it breaks this > way, Where did you report this? I believe you know how difficult it is to fix bugs that do not get reported. Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/mov: Ignore avio_skip() return value
On Thu, 26 Jan 2017 08:25:15 +0100 Carl Eugen Hoyos wrote: > 2017-01-26 6:27 GMT+01:00 wm4 : > > On Thu, 26 Jan 2017 00:34:20 +0100 > > Carl Eugen Hoyos wrote: > > > >> From 694daed9222e50d6245bf5d041e82523ee869451 Mon Sep 17 00:00:00 2001 > >> From: Carl Eugen Hoyos > >> Date: Thu, 26 Jan 2017 00:32:23 +0100 > >> Subject: [PATCH] lavf/mov: Ignore avio_skip() return value. > >> > >> Fixes ticket #6102. > >> --- > >> libavformat/mov.c |4 +--- > >> 1 file changed, 1 insertion(+), 3 deletions(-) > >> > >> diff --git a/libavformat/mov.c b/libavformat/mov.c > >> index 7dc550e..6f7e80a 100644 > >> --- a/libavformat/mov.c > >> +++ b/libavformat/mov.c > >> @@ -4794,9 +4794,7 @@ static int mov_read_uuid(MOVContext *c, AVIOContext > >> *pb, MOVAtom atom) > >> av_free(buffer); > >> } else { > >> // skip all uuid atom, which makes it fast for long uuid-xmp > >> file > >> -ret = avio_skip(pb, len); > >> -if (ret < 0) > >> -return ret; > >> +avio_skip(pb, len); > >> } > >> } else if (!memcmp(uuid, uuid_spherical, sizeof(uuid))) { > >> size_t len = atom.size - sizeof(uuid); > > > > Why would you just ignore the error? > > It fixes a regression (that I suspect will hit you very soon) and > it's what all other (17? 24?) calls to avio_skip() in mov.c do. > > Doesn't an error here indicate a hardware issue? We do not normally ignore errors, especially not hardware errors. mov.c really does contain a whole lot of unchecked skip, read and seek calls. Low code quality, I guess. Anyway, in issue #6102 you mentioned that this was caused by this commit: http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=25e35b34365ea4fc737f406992b7947a0610edcb The original code had no unchecked calls. Are you sure the commit shouldn't just reverted, instead of removing an error check? Can you explain why it's ok to ignore the error in this case specifically? Does it actually work, or just prevent read_header from returning early? Also, please provide all the information in the commit message, instead of just mentioning an issue. This is why the commit message field exists; use it. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] libavformat patch that brute-forces aax encryption
2017-01-25 3:48 GMT+01:00 William Shipley : > Is there any interest in including this upstream? Please send your patch to this mailing list. Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata
On Thu, 26 Jan 2017 08:26:02 +0100 Carl Eugen Hoyos wrote: > 2017-01-26 6:24 GMT+01:00 wm4 : > > On Thu, 26 Jan 2017 00:35:17 +0100 > > Carl Eugen Hoyos wrote: > > > >> 2017-01-26 0:19 GMT+01:00 Hendrik Leppkes : > >> > On Thu, Jan 26, 2017 at 10:04 AM, Carl Eugen Hoyos > >> > wrote: > >> >> 2017-01-25 14:22 GMT+01:00 wm4 : > >> >>> On Mon, 12 Dec 2016 12:12:37 +0100 > >> >>> Carl Eugen Hoyos wrote: > >> >>> > >> From 7c26220a8734fe7dc293efe6c13e3baf91defc7e Mon Sep 17 00:00:00 2001 > >> From: Carl Eugen Hoyos > >> Date: Mon, 12 Dec 2016 12:07:27 +0100 > >> Subject: [PATCH 2/2] lavf/mov: Export vendor metadata. > >> > >> --- > >> libavformat/mov.c |5 - > >> tests/ref/fate/mov-zombie |2 +- > >> 2 files changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/libavformat/mov.c b/libavformat/mov.c > >> index 0b1c182..a19ebbf 100644 > >> --- a/libavformat/mov.c > >> +++ b/libavformat/mov.c > >> @@ -1842,6 +1842,7 @@ static void mov_parse_stsd_video(MOVContext *c, > >> AVIOContext *pb, > >> AVStream *st, MOVStreamContext *sc) > >> { > >> uint8_t codec_name[32] = { 0 }; > >> +uint8_t vendor[5] = { 0 }; > >> int64_t stsd_start; > >> unsigned int len; > >> > >> @@ -1851,7 +1852,9 @@ static void mov_parse_stsd_video(MOVContext *c, > >> AVIOContext *pb, > >> > >> avio_rb16(pb); /* version */ > >> avio_rb16(pb); /* revision level */ > >> -avio_rb32(pb); /* vendor */ > >> +avio_read(pb, vendor, 4); > >> +if (vendor[0]) > >> +av_dict_set(&st->metadata, "vendor", vendor, 0); > >> >>> > >> >>> Does this mean transcoding to a format with per-stream > >> >>> tags will add this as a tag? > >> >> > >> >> The patch you quoted allows libavformat users to read the > >> >> vendor tag from mov files, I don't understand how it adds > >> >> tags. > >> >> Do you object? > >> > > >> > Any metadata you export can and will get copied to a new file when > >> > remuxing, therefor exporting arbitrary info that isn't actual stream > >> > metadata tags in metadata is problematic - it carries over to the > >> > destination file, in which it would be entirely meaningless. > >> > >> Sorry, I don't understand: > >> Which application do you mean? > > > > ffmpeg > > Didn't I ping yesterday a patch that avoids this? > And wasn't this the mail you answered? Sorry, I couldn't find it just now. What was the subject line? I'm not necessarily blocking your patch. There are already plenty of other cases where it breaks this way, and your patch just adds another. But maybe they can all be fixed by that patch you mentioned. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel