On Fri, 27 Jan 2017, Peter Große wrote:
On Fri, 27 Jan 2017 16:09:06 +0200 (EET)
Martin Storsjö <[email protected]> wrote:
+ // common Webm codecs are not part of RFC 6381
+ switch (par->codec_id) {
+ case AV_CODEC_ID_VP8:
+ snprintf(str, size, "vp8");
+ return;
+ case AV_CODEC_ID_VP9:
+ snprintf(str, size, "vp9");
+ return;
+ case AV_CODEC_ID_VORBIS:
+ snprintf(str, size, "vorbis");
+ return;
+ case AV_CODEC_ID_OPUS:
+ snprintf(str, size, "opus");
+ return;
+ }
+
Hmm, I'm pondering if it'd be worth to store these in some more compact
form, like a table, for all codecs which just have a single simple plain
string?
Do you have some example code? I'm not sure if I get how to do tables in C.
Like with a struct?
struct codec_string {
int codec_id;
const char *codec_str;
};
struct codec_string codecs[] = {
{AV_CODEC_ID_VP8, "vp8"},
{AV_CODEC_ID_VP9, "vp9"},
...
};
Yes, something like this. If favouring compact code (which we tend to do),
something like this would do:
static struct codec_string {
int id;
const char *str;
} codecs[] = {
{ AV_CODEC_ID_VP8, "vp8" },
...
{ 0, NULL }
};
int i;
for (i = 0; codecs[i].id; i++)
if (codecs[i].id == id)
return codecs[i].str;
All in all, the number of lines probably might end up higher than before,
but the bulk of the repetitive part is more compact, and adding another
entry is less annoying.
Using oformat->name here feels a little fragile. I think I'd rather have
that stored as a string somewhere in the context. (Not sure if it's better
to have it "flexible" to compose it as video/<string> and audio/<string>,
or store the full "video/mp4" etc as audioMime and videoMime.)
I might opt for the short version, since I use the format name also for
conditionals like
if (!strcmp(os->ctx->oformat->name, "mp4"))
Therefor maybe a char *format_name in the OutputStream struct?
That might be ok. Maybe a comment at the place where it's assigned, saying
that this will be used as part of a mime string (so future other formats
won't accidentally create bogus mime strings).
ctx->interrupt_callback = s->interrupt_callback;
ctx->opaque = s->opaque;
ctx->io_close = s->io_close;
ctx->io_open = s->io_open;
+ av_dict_copy(&ctx->metadata, s->metadata, 0);
Does the webm muxer need some specific metadata which we don't pass
through right now, or is it just for making sure that metadata ends up set
on the chained muxer and included in the stream? I'd at least want a
comment in the commit message explaining why this is necessary.
No specific metadata is needed. I can move this to a separate commit.
if (!(st = avformat_new_stream(ctx, NULL))) {
ret = AVERROR(ENOMEM);
@@ -798,7 +829,10 @@ static int dash_write_header(AVFormatContext *s)
avcodec_parameters_copy(st->codecpar, s->streams[i]->codecpar);
st->sample_aspect_ratio = s->streams[i]->sample_aspect_ratio;
st->time_base = s->streams[i]->time_base;
+ st->avg_frame_rate = s->streams[i]->avg_frame_rate;
ctx->avoid_negative_ts = s->avoid_negative_ts;
+ ctx->flags = s->flags;
+ ctx->max_delay = s->max_delay;
Are these new settings strictly necessary for chained-webm? If not, I'd
rather add them in a separate commit afterwards, with an explanation on
what it helps for.
To be honest, I haven't checked. I just looked at what other muxers having
subordinate muxers do, when they initialize the context.
Ok - absolutely in another commit then, if at all.
In general it might seem useful to pass on as many fields as possible, but
so far, I've taken the inverse approach; only forward the ones I
absolutely know I need. I probably don't mind adding this with a commit
message with a good motivation though.
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
