GNOME Bugzilla – Bug 791736
wavenc: fix tag and toc handling
Last modified: 2018-11-03 15:24:51 UTC
Created attachment 365692 [details] [review] Test cases Wavenc uses local tag list and toc which are initialized when receiving tag/toc events. Upon EOS, the tag list and toc are written and wavenc decides what to do with the tags/toc set by the user using the TAG_SETTER/TOC_SETTER interfaces. The way it worked was such that the upstream tags/toc couldn't be reseted nor replaced. The first commit introduces test cases, which show the faulty behaviours: - Receiving tags/toc from upstream. - User triggered tags/toc reset. - User triggered tags/toc replacement & tags append. The third commit modifies tags/toc handling to use TAG_SETTER/TOC_SETTER for both upstream tags/toc and user defined tags/toc.
Created attachment 365693 [details] [review] Fix memory leaks
Created attachment 365694 [details] [review] Fix tags/toc handling
Created attachment 365709 [details] [review] Fix memory leaks (+ copy/paste correction)
Review of attachment 365692 [details] [review]: Thanks, looks good in general just one comment :) ::: tests/check/elements/wavenc.c @@ +146,3 @@ +{ + guint32 result = GST_READ_UINT32_LE (info->data + *index); + (*index) += 4; Maybe use GstByteReader for these things? It does exactly what you reimplemented here :)
Review of attachment 365694 [details] [review]: ::: gst/wavenc/gstwavenc.c @@ +998,3 @@ + tag_setter = GST_TAG_SETTER (wavenc); + gst_tag_setter_merge_tags (tag_setter, tags, + gst_tag_setter_get_tag_merge_mode (tag_setter)); Tag setter and event tags should be kept separate and only be merged into a temporary taglist before writing. The event tags are transient, they would be reset when the event is shut down. Tag setter tags are forever until the application sets a different tag list
Created attachment 365742 [details] [review] Use GstByteReader in unit tests
> Tag setter and event tags should be kept separate and only be merged into a > temporary taglist before writing. The event tags are transient, they would > be reset when the event is shut down. Tag setter tags are forever until the > application sets a different tag list Got it. Will revert to previous tags list handling and fix the unit tests accordingly.
Thanks :) The same is true for toc setter btw, the toc should persist forever until the application is setting a different one
> The same is true for toc setter btw, the toc should persist > forever until the application is setting a different one I think we have an issue with current TOC Setter model: - There is no merge mode like we have for GstTagSetter. - Current implementations for wavenc and flacenc store the upstream TOC in a dedicated GstToc field. We have no way of resetting it from the application. The application TOC is ignored if an upstream TOC has been received. I would at least use the application TOC over the upstream TOC. - In matroska-mux, when a TOC event is received, the element stores it using the TOC Setter. This behaviour allows resetting the upstream TOC or replacing it when done in the proper state. However, this doesn't comply with the persistent nature of the application TOC. Did I miss something? Should we enhance GstTocSetter in order to be able to indicate some sort of TOC selection mode (I wouldn't go so far as to propose a merge mode)? This would match something like: - GST_TOC_SELECT_UPSTREAM. This would match current behaviour. IMHO it shouldn't be the default though. - GST_TOC_SELECT_APPLICATION.
On a second though, my proposition above is not good enough. If GST_TOC_SELECT_APPLICATION were the default, it wouldn't handle the case (Upstream TOC available, no Application TOC defined) properly. Maybe, we should just use a boolean flag to opt-in to prevent any TOC from being written (e.g. discard_toc). The behaviour would then be: - discard_toc to FALSE: * (!U, !A) => - * (U, !A) => U * (!U, A) => A * (U, A) => A - discard_toc to TRUE: * - (whatever the Upstream and Application TOC availability)
It sounds like the GstTocSetter interface should indeed be extended. Apart from some kind of selection, there should also be a way for the application to get in-stream TOCs (so it can potentially merge them) and be notified about them changing. Also you can get different tocs for every single sinkpad of a muxer, which does not really simplify anything here. I think an enum on the interface would make most sense for deciding what to write though
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/426.