GNOME Bugzilla – Bug 751778
metainfo: some meta has no transform_func in plugins
Last modified: 2015-08-16 13:40:59 UTC
There are some meta-info which doesn't have transform_func in some plugins. <gstreamer core> gst_protection_meta_get_info <good plugins> gst_meta_ximage_get_info <bad plugins> gst_core_media_meta_get_info gst_core_video_meta_get_info gst_meta_dfbsurface_get_info gst_mpeg_video_meta_get_info gst_pvr_meta_get_info
At least the first two and the mpegvideo one should get transform functions for the COPY transform I guess, ideally all of them if that is possible.
Created attachment 306495 [details] [review] protection: add meta transform function I'll work on other plugins continuously unless this is incorrect.
Created attachment 306498 [details] [review] mpegvideometa: add meta transform function
Comment on attachment 306495 [details] [review] protection: add meta transform function This is handled in bug #749590
(In reply to Tim-Philipp Müller from comment #4) > Comment on attachment 306495 [details] [review] [review] > protection: add meta transform function > > This is handled in bug #749590 Thanks for notify. I'm going to attach new patch for this on the bug thread.
Created attachment 306592 [details] [review] ximagesrc: add meta transfrom fucntion
Comment on attachment 306592 [details] [review] ximagesrc: add meta transfrom fucntion That doesn't seem correct, a half-filled ximage meta is not going to be useful for anybody. If it can't contain the ximage, it should just be dropped :) So make it an empty transform function
Comment on attachment 306498 [details] [review] mpegvideometa: add meta transform function I think it should return FALSE if it's not a COPY transform. The docs say: Returns: TRUE if the transform could be performed
Created attachment 306906 [details] [review] ximagesrc: add meta transfrom fucntion Following slomo's review
Created attachment 306907 [details] [review] mpegvideometa: add meta transform function Following slomo's review.
So, it seems like all meta transform functions are wrong currently :) They should return FALSE when transformation fails (they do), and also return FALSE when the transform type is not known (they don't). Basically in all cases where nothing was done successfully they should return FALSE. Want to provide patches and update yours?
Created attachment 306970 [details] [review] mpegvideometa: add meta transform function
Created attachment 306971 [details] [review] meta: transform_func: return FALSE if not supported or failed
Created attachment 306972 [details] [review] video/audio meta: transform_func: return FALSE if not supported or failed
Created attachment 306973 [details] [review] glsyncmeta: transform func: return FALSE if not supported or failed
Review of attachment 306970 [details] [review]: ::: gst-libs/gst/codecparsers/gstmpegvideometa.c @@ +90,3 @@ + } + + return TRUE; No, this should return FALSE in both cases here: - if gst_buffer_add_mpeg_video_meta() fails - if the transform type is not handled All cases other than the one where adding the meta succeeded should return FALSE. Same goes for all the other patches
Comment on attachment 306970 [details] [review] mpegvideometa: add meta transform function Nevermind, I fixed that now ;)
Review of attachment 306972 [details] [review]: ::: gst-libs/gst/audio/gstaudiometa.c @@ +64,3 @@ smeta = (GstAudioDownmixMeta *) meta; + + dmeta = gst_buffer_add_audio_downmix_meta (dest, smeta->from_position, Not your fault, but this transform function seems wrong. It should only copy the meta if the transform type is COPY. E.g. if this goes through audioconvert and changes channel numbers, then it shouldn't just be copied probably.
(In reply to Sebastian Dröge (slomo) from comment #17) > Comment on attachment 306970 [details] [review] [review] > mpegvideometa: add meta transform function > > Nevermind, I fixed that now ;) Oops. Thanks slomo :)
Created attachment 307011 [details] [review] video/audio meta: transform_func: return FALSE if not supported or failed Yes. It would be problem without checking transform type as you say.
Thanks, all pushed :) Now only the protectionmeta is missing?
(In reply to Sebastian Dröge (slomo) from comment #21) > Thanks, all pushed :) Now only the protectionmeta is missing? Yes it's handled on https://bugzilla.gnome.org/show_bug.cgi?id=749590 as tim said. But the patch on the bug needs to add code to handle failure.