Comment on attachment 321701 [details] [review]
Patch

I think this generally makes sense, what use are empty tags?

But if I understand this correctly, you would still write empty tags if there are also some non-empty ones? Is that intentional? Just skips the non-empty ones :)


And why are you checking against gst_value_serialize() being NULL? That doesn't seem like the ideal check for empty tags

Not quite sure what you mean by that. By empty tag, I meant something that gst_matroska_mux_write_simple_tag ignores. That means tags that can not be mapped (by gst_matroska_tag_conv) or tags with value that failed to serialize. Those are the tags that GST_MATROSKA_ID_SIMPLETAG will not be written for.
Hence the NULL check after serialization. 

So if you have bunch of tags, and not a single one of them maps or serializes, you can not write the surrounding GST_MATROSKA_ID_TAGS and GST_MATROSKA_ID_TAG element, because webm parser chokes on empty GST_MATROSKA_ID_TAG element.

Using gst_matroska_mux_tag_list_is_empty instead of gst_tag_list_is_empty should take care of this (look at for example gst_matroska_mux_start).

That said, I'm not entirely sure if tags with empty (zero length) value would also break the parser. I'll test it, but looking at the mkvparser.cpp code, I actually think it might, because ParseElementHeader is also being used to parse GST_MATROSKA_ID_TAGSTRING elements; If that's the case, we'll probably need to check for serialized value both being null and empty string.

So the problem is empty TAGS without containing a TAG? Or TAG without content? We shouldn't write either of these IMHO.

But testing if a tag is empty via serialization returning NULL seems a bit weird nonetheless.

well, the way tags are muxed in matroska is TAGS -> TAG -> bunch of SIMPLE_TAGs. 

The issue is TAG without content (no SIMPLE_TAGs); And I'm also guessing SIMPLE_TAG with empty string value too, I'll have to test later.

The null check after serialization is same thing gst_matroska_mux_write_simple_tag does. I wanted it to be consistent. gst_matroska_mux_write_simple_tag skips tags that have value that fails to serialize, so gst_matroska_mux_tag_list_is_empty should do the same.

So I think we should not add empty SIMPLE_TAGS, and not TAGS and TAG if they are not going to contain anything. Everything else is just useless information in the files anyway :)

My main remaining problem here is that we now serialize each tag twice but I don't see how that can be easily improved in a way that is not even more ugly. You? :)

Two things: Tags that serialize to empty string are handled correctly. So we don't need to skip such tags (it actually wouldn't seem right anyway). Thus the patch should cover all tag related scenarios.

> So I think we should not add empty SIMPLE_TAGS, and not TAGS and TAG if they are not going to contain anything

That's what my patch does. If none of the SIMPLE_TAGs are be written, the outer TAGS and TAG will not be written either. Otherwise the file won't load.

Serializing the tag twice may not be the most elegant solution, but needs to be done for consistency. gst_matroska_mux_write_simple_tag skips tags that won't serialize, so we need to know upfront if that's the case before writing TAGS and TAG.

commit 8657987f8f9e659a7207f5d69fdc679f5ff8f24e
Author: Matej Knopp <matej.knopp@gmail.com>
Date:   Fri Feb 19 23:44:42 2016 +0100

    matroskamux: don't output empty tags/tag elements
    
    Such files will not play on Android, because of bug in libwebm matroska parsing, which is still present in 6.0.1
    
    https://bugzilla.gnome.org/show_bug.cgi?id=762349

Let's go with that then for now.

*** Bug 738575 has been marked as a duplicate of this bug. ***