On Thu, 31 Jul 2014, John Stebbins wrote:
On 07/31/2014 12:42 PM, Martin Storsjö wrote:
On Thu, 31 Jul 2014, John Stebbins wrote:
---
libavformat/movenc.c | 77 ++++++++++++++++++++++++++++++++++++++++++++--------
libavformat/movenc.h | 1 +
2 files changed, 67 insertions(+), 11 deletions(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index f16e851..b384f4d 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -77,6 +77,17 @@ static const AVClass flavor ## _muxer_class = {\
.version = LIBAVUTIL_VERSION_INT,\
};
+static int utf8len(const uint8_t *b)
+{
+ int len = 0;
+ int val;
+ while (*b) {
+ GET_UTF8(val, *b++, return -1;)
+ len++;
+ }
+ return len;
+}
+
//FIXME support 64 bit variant with wide placeholders
static int64_t update_size(AVIOContext *pb, int64_t pos)
{
@@ -1352,6 +1363,16 @@ 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);
}
+ if (track->st) {
+ // hdlr.name is used by some players to identify the content title
+ // of the track. So if an alternate handler description is
+ // specified, use it.
+ AVDictionaryEntry *t;
+ t = av_dict_get(track->st->metadata, "handler", NULL, 0);
+ if (t && utf8len(t->value)) {
+ descr = t->value;
+ }
+ }
}
avio_wb32(pb, 0); /* size */
@@ -1671,6 +1692,48 @@ static int mov_write_udta_sdp(AVIOContext *pb, MOVTrack
*track)
return len + 24;
}
+static int mov_write_track_metadata(AVIOContext *pb, AVStream *st,
+ const char *tag, const char *str)
+{
+ int64_t pos = avio_tell(pb);
+ AVDictionaryEntry *t = av_dict_get(st->metadata, str, NULL, 0);
+ if (!t || !utf8len(t->value))
+ return 0;
+
+ avio_wb32(pb, 0); /* size */
+ ffio_wfourcc(pb, tag); /* type */
+ avio_write(pb, t->value, strlen(t->value)); /* UTF8 string value */
+ return update_size(pb, pos);
+}
+
+static int mov_write_track_udta_tag(AVIOContext *pb, MOVMuxContext *mov,
+ AVStream *st)
+{
+ AVIOContext *pb_buf;
+ int ret, size;
+ uint8_t *buf;
+
+ if (st == NULL || mov->fc->flags & AVFMT_FLAG_BITEXACT)
+ return 0;
Hmm, why not write this if wanting bitexact output? Writing some metadata
fields is bitexact, no? We normally only discard e.g. lavf version
identifiers and other things which change often or are nondeterministic. I
guess that the fate checksum changes if you remove this - but in that case
just update the fate checksum instead.
It was unclear to me what needs to be disabled for BITEXACT. So I mimicked
what is done in mov_write_udta_tag. If this
isn't necessary, it can certainly be removed.
Hmm, didn't know we did that. Then it's probably ok for now to mimic that
- if we think it should be changed we can change it for all cases at once.
+
+ ret = avio_open_dyn_buf(&pb_buf);
+ if (ret < 0)
+ return ret;
+
+ if (mov->mode & MODE_MP4) {
+ mov_write_track_metadata(pb_buf, st, "name", "title");
Umm, this literally just writes "title" here - but where is the title
written then? Or is there something that I'm missing? Is this related to
writing the metadata "handler" key above, or is that a separate thing in
the same patch?
It looks up the value of "title" in st->metadata and writes a box with type
"name" and payload value.
Sorry, I completely missed the av_dict_get part in
mov_write_track_metadata. Nevermind about this comment then.
This chunk then gets output as the body of the udta box for this track
(if the size of the chunk is > 0). It has nothing to do with hdlr or
the new "handler" key. If the user wants to support both track naming
conventions they must set both "title" and "handler" in track metadata.
VLC, QT, and others use udta in the trak box for track titles. MPC,
mediainfo, and probably other players based on gpac use hdlr description
field for track titles. The use of hdlr description for the track title
is broken IMO, but there are players that use it. Using a separate
metadata key for this allows you to *not* use this questionable method
of naming tracks if desired.
+ }
If mode isn't MODE_MP4, we just write an empty udta atom - that doesn't
feel too useful - shouldn't we just skip writing it altogether in that
case?
If the size of the metadata that is written is 0, the udta box is not written.
Ah, right.
We could move the MP4 test earlier before starting the udta box, but I
feel it makes the code less easily extensible. udta boxes are
permissible in other modes, we just haven't added any code yet to fill
one in here.
Indeed, when looking at the existing mov_write_udta_tag, this makes more
sense.
+
+ if ((size = avio_close_dyn_buf(pb_buf, &buf)) > 0) {
Why the indirection via the separate AVIOContext? You can write multiple
nested atoms without any indirection, just do:
int64_t pos = avio_tell(pb);
avio_wb32(pb, 0);
ffio_wfourcc(pb, "udta");
mov_write_track_metadata();
update_size(pb, pos1);
See above. It is checking if the size written is 0 and skipping writing an
empty udta box. It could certainly be done
a different way. I chose the easy way which was to cut and paste code from
mov_write_udta_tag and make some relatively
minor modifications.
Ok then.
So all in all, no objections from me about this patch any longer.
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
