On Wed, 2 Jul 2014, Luca Barbato wrote:
---
libavformat/movenc.c | 57 +++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 47 insertions(+), 10 deletions(-)
For the subject, please reorder the sentence, "Make format-specific
details settable"
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index f5c36fc..713949a 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -1314,11 +1314,16 @@ static int mov_write_hdlr_tag(AVIOContext *pb, MOVTrack
*track)
const char *hdlr, *descr = NULL, *hdlr_type = NULL;
int64_t pos = avio_tell(pb);
+ uint32_t flags = 0, mask = 0;
+
hdlr = "dhlr";
hdlr_type = "url ";
descr = "DataHandler";
if (track) {
+ AVStream *st = track->st;
+ AVDictionaryEntry *tag;
+
hdlr = (track->mode == MODE_MOV) ? "mhlr" : "\0\0\0\0";
if (track->enc->codec_type == AVMEDIA_TYPE_VIDEO) {
hdlr_type = "vide";
@@ -1352,6 +1357,13 @@ static int mov_write_hdlr_tag(AVIOContext *pb, MOVTrack
*track)
"Unknown hldr_type for %s / 0x%04X, writing dummy values\n",
tag_buf, track->enc->codec_tag);
}
+ tag = av_dict_get(st->metadata, "mov-component-flags", NULL, 0);
+ if (tag)
+ flags = atoi(tag->value);
+
+ tag = av_dict_get(st->metadata, "mov-component-mask", NULL, 0);
+ if (tag)
+ mask = atoi(tag->value);
}
avio_wb32(pb, 0); /* size */
@@ -1360,8 +1372,8 @@ static int mov_write_hdlr_tag(AVIOContext *pb, MOVTrack
*track)
avio_write(pb, hdlr, 4); /* handler */
ffio_wfourcc(pb, hdlr_type); /* handler type */
avio_wb32(pb, 0); /* reserved */
- avio_wb32(pb, 0); /* reserved */
- avio_wb32(pb, 0); /* reserved */
+ avio_wb32(pb, flags);
+ avio_wb32(pb, mask);
if (!track || track->mode == MODE_MOV)
avio_w8(pb, strlen(descr)); /* pascal string */
avio_write(pb, descr, strlen(descr)); /* handler description */
@@ -1464,6 +1476,12 @@ static int mov_write_tkhd_tag(AVIOContext *pb, MOVTrack
*track, AVStream *st)
int64_t duration = av_rescale_rnd(track->track_duration, MOV_TIMESCALE,
track->timescale, AV_ROUND_UP);
int version = duration < INT32_MAX ? 0 : 1;
+ int flags = (track->flags & MOV_TRACK_ENABLED) ?
+ MOV_TKHD_FLAG_ENABLED | MOV_TKHD_FLAG_IN_MOVIE :
+ MOV_TKHD_FLAG_IN_MOVIE;
This could do with some extra deobfuscation (separately after this patch),
into something like this, which would be way more readable to me:
int flags = MOV_TKHD_FLAG_IN_MOVIE;
if (track->flags & MOV_TRACK_ENABLED)
flags |= MOV_TKHD_FLAG_ENABLED;
+ int group = st ? st->codec->codec_type : 0;
+
+ AVDictionaryEntry *tag;
if (track->mode == MODE_ISM)
version = 1;
@@ -1471,9 +1489,13 @@ static int mov_write_tkhd_tag(AVIOContext *pb, MOVTrack
*track, AVStream *st)
(version == 1) ? avio_wb32(pb, 104) : avio_wb32(pb, 92); /* size */
ffio_wfourcc(pb, "tkhd");
avio_w8(pb, version);
- avio_wb24(pb, (track->flags & MOV_TRACK_ENABLED) ?
- MOV_TKHD_FLAG_ENABLED | MOV_TKHD_FLAG_IN_MOVIE :
- MOV_TKHD_FLAG_IN_MOVIE);
+
+ tag = av_dict_get(st->metadata, "mov-flags", NULL, 0);
+ if (tag)
+ flags = atoi(tag->value);
Hmm, this feels a bit more risky to be able to overwrite. Not sure if it
would make sense to do flags |= atoi() here, although if we allow
overriding it, perhaps we should allow overriding all of it (and I guess
you might need that for your usecase). So I guess it's ok - the risk of
accidentally breaking a file by the mov-flags metadata key being present
in per-stream metadata is pretty small.
+
+ avio_wb24(pb, flags);
+
if (version == 1) {
avio_wb64(pb, track->time);
avio_wb64(pb, track->time);
@@ -1490,13 +1512,28 @@ static int mov_write_tkhd_tag(AVIOContext *pb, MOVTrack
*track, AVStream *st)
avio_wb32(pb, 0); /* reserved */
avio_wb32(pb, 0); /* reserved */
- avio_wb16(pb, 0); /* layer */
- avio_wb16(pb, st ? st->codec->codec_type : 0); /* alternate group) */
- /* Volume, only for audio */
- if (track->enc->codec_type == AVMEDIA_TYPE_AUDIO)
- avio_wb16(pb, 0x0100);
+ tag = av_dict_get(st->metadata, "mov-layer", NULL, 0);
+ if (tag)
+ avio_wb16(pb, atoi(tag->value)); /* layer */
else
+ avio_wb16(pb, 0); /* layer */
I think this would be more readable if you'd declare a variable for it,
and do all the av_dict_get() at one place, and just write the variables
here
+
+ tag = av_dict_get(st->metadata, "mov-alternate-group", NULL, 0);
+ if (tag)
+ group = atoi(tag->value);
+ group = st->index;
+ avio_wb16(pb, group); /* alternate group) */
+
+ /* Volume, only for audio */
+ if (track->enc->codec_type == AVMEDIA_TYPE_AUDIO) {
+ tag = av_dict_get(st->metadata, "mov-volume", NULL, 0);
+ if (tag)
+ avio_wb16(pb, atoi(tag->value));
+ else
+ avio_wb16(pb, 0x0100);
+ } else {
avio_wb16(pb, 0);
+ }
avio_wb16(pb, 0); /* reserved */
/* Matrix structure */
--
1.9.0
All in all, it looks acceptable I think - does Anton want to comment on it
as well? This sure is a fringe usecase, but the patch doesn't seem too
harmful to me.
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
