On Fri, 12 Dec 2014 02:24:54 +0100
Michael Niedermayer <michae...@gmx.at> wrote:
> On Thu, Dec 11, 2014 at 01:18:21AM +0000, Vittorio Giovara wrote:
> > On Wed, Dec 10, 2014 at 1:23 PM, wm4 <nfx...@googlemail.com> wrote:
> > > On Wed, 10 Dec 2014 03:38:03 +0100
> > > Lukasz Marek <lukasz.m.lu...@gmail.com> wrote:
> > >
> > >> W dniu środa, 10 grudnia 2014 Vittorio Giovara
> > >> <vittorio.giov...@gmail.com>
> > >> napisał(a):
> > >>
> > >> > On Tue, Dec 9, 2014 at 10:17 PM, wm4 <nfx...@googlemail.com
> > >> > <javascript:;>>
> > >> > wrote:
> > >> > > On Tue, 9 Dec 2014 14:10:22 +0100
> > >> > > Michael Niedermayer <michae...@gmx.at <javascript:;>> wrote:
> > >> > >
> > >> > >> TODO: bump version, update APIChanges
> > >> > >>
> > >> > >> Signed-off-by: Michael Niedermayer <michae...@gmx.at <javascript:;>>
> > >> > >> ---
> > >> > >> libavformat/avformat.h | 8 ++++++++
> > >> > >> libavformat/dump.c | 24 ++++++++++++++++++------
> > >> > >> libavformat/options_table.h | 1 +
> > >> > >> 3 files changed, 27 insertions(+), 6 deletions(-)
> > >> > >>
> > >> > >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> > >> > >> index 2e54ed1..cbe3608 100644
> > >> > >> --- a/libavformat/avformat.h
> > >> > >> +++ b/libavformat/avformat.h
> > >> > >> @@ -1616,6 +1616,14 @@ typedef struct AVFormatContext {
> > >> > >> */
> > >> > >> char *format_whitelist;
> > >> > >>
> > >> > >> + /**
> > >> > >> + * Maximum number of lines per metadata tag to dump with
> > >> > >> av_log.
> > >> > >> + * -1 means default
> > >> > >> + * - encoding: unused
> > >> > >> + * - decoding: set by user through AVOptions (NO direct access)
> > >> > >> + */
> > >> > >> + int dump_metadata_lines;
> > >> > >> +
> > >> > >>
> > >> > >> /*****************************************************************
> > >> > >> * All fields below this line are not part of the public API.
> > >> > >> They
> > >> > >> * may not be used outside of libavformat and can be changed
> > >> > >> and
> > >> > >> diff --git a/libavformat/dump.c b/libavformat/dump.c
> > >> > >> index 56b37ff..38286b8 100644
> > >> > >> --- a/libavformat/dump.c
> > >> > >> +++ b/libavformat/dump.c
> > >> > >> @@ -126,7 +126,7 @@ static void print_fps(double d, const char
> > >> > >> *postfix)
> > >> > >> av_log(NULL, AV_LOG_INFO, "%1.0fk %s", d / 1000, postfix);
> > >> > >> }
> > >> > >>
> > >> > >> -static void dump_metadata(void *ctx, AVDictionary *m, const char
> > >> > *indent)
> > >> > >> +static void dump_metadata(void *ctx, AVDictionary *m, const char
> > >> > *indent, int dump_metadata_lines_arg)
> > >> > >> {
> > >> > >> if (m && !(av_dict_count(m) == 1 && av_dict_get(m, "language",
> > >> > NULL, 0))) {
> > >> > >> AVDictionaryEntry *tag = NULL;
> > >> > >> @@ -135,16 +135,28 @@ static void dump_metadata(void *ctx,
> > >> > >> AVDictionary
> > >> > *m, const char *indent)
> > >> > >> while ((tag = av_dict_get(m, "", tag,
> > >> > >> AV_DICT_IGNORE_SUFFIX)))
> > >> > >> if (strcmp("language", tag->key)) {
> > >> > >> const char *p = tag->value;
> > >> > >> + int lines = 0;
> > >> > >> + int dump_metadata_lines = dump_metadata_lines_arg;
> > >> > >> + if (dump_metadata_lines == -1) {
> > >> > >> + dump_metadata_lines = strcmp("comment",
> > >> > >> tag->key)
> > >> > ? 1 : 25;
> > >> > >> + }
> > >> > >> av_log(ctx, AV_LOG_INFO,
> > >> > >> "%s %-16s: ", indent, tag->key);
> > >> > >> while (*p) {
> > >> > >> char tmp[256];
> > >> > >> size_t len = strcspn(p, "\x8\xa\xb\xc\xd");
> > >> > >> + if (lines >= dump_metadata_lines) {
> > >> > >> + av_log(ctx, AV_LOG_INFO,
> > >> > >> "[%"SIZE_SPECIFIER"
> > >> > bytes ommited, use \'-dump_metadata_lines <max>\' to see more]",
> > >> > strlen(p));
> > >> > >> + break;
> > >> > >> + }
> > >> > >> av_strlcpy(tmp, p, FFMIN(sizeof(tmp), len+1));
> > >> > >> av_log(ctx, AV_LOG_INFO, "%s", tmp);
> > >> > >> p += len;
> > >> > >> if (*p == 0xd) av_log(ctx, AV_LOG_INFO, " ");
> > >> > >> - if (*p == 0xa) av_log(ctx, AV_LOG_INFO, "\n%s
> > >> > %-16s: ", indent, "");
> > >> > >> + if (*p == 0xa) {
> > >> > >> + av_log(ctx, AV_LOG_INFO, "\n%s %-16s: ",
> > >> > indent, "");
> > >> > >> + lines++;
> > >> > >> + }
> > >> > >> if (*p) p++;
> > >> > >> }
> > >> > >> av_log(ctx, AV_LOG_INFO, "\n");
> > >> > >> @@ -420,7 +432,7 @@ static void dump_stream_format(AVFormatContext
> > >> > >> *ic,
> > >> > int i,
> > >> > >> av_log(NULL, AV_LOG_INFO, " (clean effects)");
> > >> > >> av_log(NULL, AV_LOG_INFO, "\n");
> > >> > >>
> > >> > >> - dump_metadata(NULL, st->metadata, " ");
> > >> > >> + dump_metadata(NULL, st->metadata, " ",
> > >> > >> ic->dump_metadata_lines);
> > >> > >>
> > >> > >> dump_sidedata(NULL, st, " ");
> > >> > >> }
> > >> > >> @@ -438,7 +450,7 @@ void av_dump_format(AVFormatContext *ic, int
> > >> > >> index,
> > >> > >> index,
> > >> > >> is_output ? ic->oformat->name : ic->iformat->name,
> > >> > >> is_output ? "to" : "from", url);
> > >> > >> - dump_metadata(NULL, ic->metadata, " ");
> > >> > >> + dump_metadata(NULL, ic->metadata, " ",
> > >> > >> ic->dump_metadata_lines);
> > >> > >>
> > >> > >> if (!is_output) {
> > >> > >> av_log(NULL, AV_LOG_INFO, " Duration: ");
> > >> > >> @@ -480,7 +492,7 @@ void av_dump_format(AVFormatContext *ic, int
> > >> > >> index,
> > >> > >> av_log(NULL, AV_LOG_INFO,
> > >> > >> "end %f\n", ch->end * av_q2d(ch->time_base));
> > >> > >>
> > >> > >> - dump_metadata(NULL, ch->metadata, " ");
> > >> > >> + dump_metadata(NULL, ch->metadata, " ",
> > >> > ic->dump_metadata_lines);
> > >> > >> }
> > >> > >>
> > >> > >> if (ic->nb_programs) {
> > >> > >> @@ -490,7 +502,7 @@ void av_dump_format(AVFormatContext *ic, int
> > >> > >> index,
> > >> > >> "name", NULL, 0);
> > >> > >> av_log(NULL, AV_LOG_INFO, " Program %d %s\n",
> > >> > ic->programs[j]->id,
> > >> > >> name ? name->value : "");
> > >> > >> - dump_metadata(NULL, ic->programs[j]->metadata, " ");
> > >> > >> + dump_metadata(NULL, ic->programs[j]->metadata, " ",
> > >> > ic->dump_metadata_lines);
> > >> > >> for (k = 0; k < ic->programs[j]->nb_stream_indexes;
> > >> > >> k++) {
> > >> > >> dump_stream_format(ic,
> > >> > ic->programs[j]->stream_index[k],
> > >> > >> index, is_output);
> > >> > >> diff --git a/libavformat/options_table.h
> > >> > >> b/libavformat/options_table.h
> > >> > >> index 40f1e0a..75ddf9c 100644
> > >> > >> --- a/libavformat/options_table.h
> > >> > >> +++ b/libavformat/options_table.h
> > >> > >> @@ -99,6 +99,7 @@ static const AVOption avformat_options[] = {
> > >> > >> {"dump_separator", "set information dump field separator",
> > >> > OFFSET(dump_separator), AV_OPT_TYPE_STRING, {.str = ", "}, CHAR_MIN,
> > >> > CHAR_MAX, D|E},
> > >> > >> {"codec_whitelist", "List of decoders that are allowed to be used",
> > >> > OFFSET(codec_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },
> > >> > CHAR_MIN,
> > >> > CHAR_MAX, D },
> > >> > >> {"format_whitelist", "List of demuxers that are allowed to be
> > >> > >> used",
> > >> > OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },
> > >> > CHAR_MIN,
> > >> > CHAR_MAX, D },
> > >> > >> +{"dump_metadata_lines", "Maximum number of lines per metadata tag
> > >> > >> to
> > >> > dump with av_log", OFFSET(dump_metadata_lines), AV_OPT_TYPE_INT, {
> > >> > .i64 =
> > >> > -1 }, -1, INT_MAX, D },
> > >> > >> {NULL},
> > >> > >> };
> > >> > >>
> > >> > >
> > >> > > Stuff liken this doesn't belong in a library. /nag
> > >> >
> > >> > +1
> > >> >
> > >>
> > >> +4
> > >> have I won?
> > >>
> > >> Sometimes I read random commits and comments. sometimes I read comments
> > >> for
> > >> my own patches. comments like last 2 one are useless, like spam. you dont
> > >> like, say why!
> > >
> > > Because dumping crap to stdio has no place in a library. What does
> > > ffprobe exist for?
> > >
> > > Sure, it's pretty isolated in dump.c, but I'm mostly offended that
> > > another obscure, barely useful (or actually completely useless) field
> > > it added to the already bloated beyond help AVFormatContext.
> >
> > I am tempted to go for a +1, but I'll go with a more lengthy "I agree
> > with wm4, in my opinion adding fields to avformatcontext for something
> > that the application should do (in this case ffmpeg) is not
> > appropriate."
>
> The problem with this philosophy is that multiple applications perform
> the task of printing information about the streams, which include
> metadata.
> In our git alone there are 9 independant applications (6 of them
> in doc/examples) the other 3 are ffmpeg, ffplay and ffprobe, which
> use this code.
>
> And code that many applications need should be shared.
> Sharing the code allows it to be extended and fixed at a single place
> also it makes the output of the applications more consistent.
> consistency in the output between applications also should help
> users fiding the information they look for quicker in an app they
> do not know so well when it has the same output as another they do
> know well
So just put printing into ffprobe, and then always use ffprobe to
inspect files, instead of duplicating the code everywhere?
If you really want to reduce rampant code duplication, improve the API
so that you don't need tons of boilerplate to do ANYTHING. Basically,
most code examples should be tiny and simple, instead of big and buggy
like they are now.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
