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.
>> +
>> + 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. 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.
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.
>> +
>> + 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.
--
John GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
signature.asc
Description: OpenPGP digital signature
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
