GNOME Bugzilla – Bug 752362
taglist: update merge logic when one of the input is empty
Last modified: 2018-11-03 12:29:07 UTC
When tag_list_merge is called with either of the input lists empty, right now, a new empty list is being created and on top of that the other list is being merged. But if one of the input is empty, it is supposed to just create a copy of the other as the merged list. This was added to handle GST_TAG_MERGE_KEEP_ALL and GST_TAG_MERGE_REPLACE_ALL mode during merging. But these modes should be valid only when both the inputs are not NULL. It doesn't make sense to create a new empty list, just to handle these two modes, even though NULL is being passed as input.
Created attachment 307393 [details] [review] update merge logic
Created attachment 307394 [details] [review] update tests for merge logic
Not sure I follow. Commit f28a2a6c explicitly changes this from what you propose to what it is today. If I understand you correctly you're basically changing the merge behaviour back, claiming that it doesn't make sense and "is supposed to just create a copy of the other" list. Why is it supposed to do that? Perhaps it's just the documentation that needs updating?
I think, it is not the job of gsttaglist, to create a new empty list whenever the caller passes NULL list, just to accommodate these two modes. If the callers needs to handle, GST_TAG_MERGE_KEEP_ALL and GST_TAG_MERGE_REPLACE_ALL, then caller should make sure there is no NULL list being passed by creating a new empty list and passing it on. This is what is happening right now. gst_tag_list_merge(list, NULL, GST_TAG_MERGE_REPLACE_ALL); list2 = gst_tag_list_new_empty (); gst_tag_list_insert(list, list2, GST_TAG_MERGE_REPLACE_ALL); If the caller expects gsttaglist to create a new empty list, even when NULL list is being passed as input, then it might as well expect gst_tag_list_insert to do the same... something like the below statement to work. gst_tag_list_insert(list, NULL, GST_TAG_MERGE_REPLACE_ALL); so in this case, since replace all is being set, then the list will be replaced with NULL.. IMO, merge should work only when there are two lists to merge, similar to how merge works in general...
If we want to maintain the same behavior, which i believe is wrong :) then we might have to change the logic of gst_tag_setter_merge_tags() to accommodate NULL list being passed on as input. But again i don't believe it is proper..
Tim, any update on this? should we maintain the behaviour and change the documentation? This means changing behaviour in tagsetter to match with taglist.
I don't know yet, but I want to leave things as they are for 1.6 and revisit it later, unless there is a convincing and real use case where this change is needed and makes sense.
ping :)
-- 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/gstreamer/issues/123.