Quoting Alexandra Hájková (2015-03-05 16:33:29)
>
> -static int asf_probe(AVProbeData *pd)
> +static int asf_read_metadata(AVFormatContext *s, const char *title, uint16_t
> len,
> + unsigned char *ch, uint16_t max_len)
Broken alignment.
> {
> - /* check file header */
> - if (!ff_guidcmp(pd->buf, &ff_asf_header))
> - return AVPROBE_SCORE_MAX;
> - else
> - return 0;
> + AVIOContext *pb = s->pb;
> +
> + memset(ch, 0, max_len);
This is unnecessary.
> + avio_get_str16le(pb, len * sizeof(*ch), ch, len * sizeof(*ch));
sizeof(*ch) is silly, it's defined by the C99 specification to be 1.
The last parameter to avio_get_str16le() should also probably be
max_len.
> +static int asf_read_metadata_obj(AVFormatContext *s, const GUIDParseTable *g)
> +{
> + ASFContext *asf = s->priv_data;
> + AVIOContext *pb = s->pb;
> + AVDictionary **met = NULL;
> + uint64_t size = avio_rl64(pb);
> + uint16_t nb_recs = avio_rl16(pb); // number of records in the
> Description Records list
> + AVStream *st = NULL;
> + int i, j, ret;
> +
> + for (i = 0; i < nb_recs; i++) {
> + uint16_t name_len, type, val_len, st_num = 0;
> + uint8_t *name = NULL;
> +
> + avio_skip(pb, 2); // skip reserved field
> + st_num = avio_rl16(pb);
> + name_len = avio_rl16(pb);
> + if (!name_len)
> + break;
> + type = avio_rl16(pb);
> + val_len = avio_rl32(pb);
> + name = av_malloc(name_len);
> + if (!name)
> + return AVERROR(ENOMEM);
> + avio_get_str16le(pb, name_len * sizeof(*name), name,
> + name_len);
> + for (j = 0; j < asf->nb_streams; j++) {
> + if (asf->asf_st[j]->stream_index == st_num) {
> + st = s->streams[asf->asf_st[j]->index];
> + break;
> + }
This seems to be done often enough to warrant factoring into a separate
function.
> }
> - goto finish;
> - } else if (type > 1 && type <= 5) { // boolean or DWORD or QWORD or WORD
> - uint64_t num = get_value(s->pb, type, type2_size);
> - snprintf(value, len, "%"PRIu64, num);
> - } else if (type == 6) { // (don't) handle GUID
> - av_log(s, AV_LOG_DEBUG, "Unsupported GUID value in tag %s.\n", key);
> - goto finish;
> - } else {
> - av_log(s, AV_LOG_DEBUG,
> - "Unsupported value type %d in tag %s.\n", type, key);
> - goto finish;
> + if (st)
> + met = &st->metadata;
> + else
> + met = &s->metadata;
Metadata referring to an unknown stream should cause to scream an error
and either skip it or abort. Writing it to some random other metadata is
a bad idea.
> + if ((ret = process_metadata(s, name, val_len, type, met)) < 0)
> + break;
> }
> - if (*value)
> - av_dict_set(&s->metadata, key, value, 0);
>
> -finish:
> - av_freep(&value);
> - avio_seek(s->pb, off + len, SEEK_SET);
> + align_position(pb, asf->offset, size);
> + return 0;
> }
>
> -static int asf_read_file_properties(AVFormatContext *s, int64_t size)
> +static int asf_read_content(AVFormatContext *s, const GUIDParseTable *g)
'content is not a good abbreviation of 'content description'
Same holds for many other object reading functions.
> {
> ASFContext *asf = s->priv_data;
> AVIOContext *pb = s->pb;
> + int i;
> + static const char *const titles[] =
> + { "Title", "Author", "Copyright", "Description", "Rate" };
> + uint16_t len[5], max_len = 0;
> + uint8_t *ch;
> + uint64_t size = avio_rl64(pb);
> +
> + for (i = 0; i < 5; i++) {
> + len[i] = avio_rl16(pb);
> + max_len = FFMAX(max_len, len[i]);
> + }
> + if (max_len) {
> + ch = av_malloc(max_len * sizeof(*ch));
> + if (!ch)
> + return(AVERROR(ENOMEM));
> + for (i = 0; i < 5; i++) {
> + asf_read_metadata(s, titles[i], len[i], ch, max_len);
The length considerations looks wrong, the length of the resulting UTF-8
string is not necessarily less than the size of the string stored in the
file. Same for extended conte description and metadata objects. You're
also consitently forgetting to allocate the extra byte for the
terminator.
Also overall, the usage of get_asf_string() vs avio_get_str16le() in
this file looks somewhat schisophrenic to me.
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
