After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 751778 - metainfo: some meta has no transform_func in plugins
metainfo: some meta has no transform_func in plugins
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-07-01 11:58 UTC by Hyunjun Ko
Modified: 2015-08-16 13:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
protection: add meta transform function (1.46 KB, patch)
2015-07-01 13:01 UTC, Hyunjun Ko
none Details | Review
mpegvideometa: add meta transform function (1.97 KB, patch)
2015-07-01 13:19 UTC, Hyunjun Ko
none Details | Review
ximagesrc: add meta transfrom fucntion (1.82 KB, patch)
2015-07-02 08:44 UTC, Hyunjun Ko
none Details | Review
ximagesrc: add meta transfrom fucntion (1.42 KB, patch)
2015-07-06 11:19 UTC, Hyunjun Ko
committed Details | Review
mpegvideometa: add meta transform function (2.00 KB, patch)
2015-07-06 11:19 UTC, Hyunjun Ko
none Details | Review
mpegvideometa: add meta transform function (2.06 KB, patch)
2015-07-07 02:25 UTC, Hyunjun Ko
committed Details | Review
meta: transform_func: return FALSE if not supported or failed (2.85 KB, patch)
2015-07-07 02:26 UTC, Hyunjun Ko
committed Details | Review
video/audio meta: transform_func: return FALSE if not supported or failed (4.92 KB, patch)
2015-07-07 02:26 UTC, Hyunjun Ko
none Details | Review
glsyncmeta: transform func: return FALSE if not supported or failed (865 bytes, patch)
2015-07-07 02:27 UTC, Hyunjun Ko
committed Details | Review
video/audio meta: transform_func: return FALSE if not supported or failed (5.18 KB, patch)
2015-07-07 13:31 UTC, Hyunjun Ko
committed Details | Review

Description Hyunjun Ko 2015-07-01 11:58:41 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
Comment 1 Sebastian Dröge (slomo) 2015-07-01 12:01:10 UTC
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.
Comment 2 Hyunjun Ko 2015-07-01 13:01:29 UTC
Created attachment 306495 [details] [review]
protection: add meta transform function

I'll work on other plugins continuously unless this is incorrect.
Comment 3 Hyunjun Ko 2015-07-01 13:19:37 UTC
Created attachment 306498 [details] [review]
mpegvideometa: add meta transform function
Comment 4 Tim-Philipp Müller 2015-07-01 13:47:47 UTC
Comment on attachment 306495 [details] [review]
protection: add meta transform function

This is handled in bug #749590
Comment 5 Hyunjun Ko 2015-07-02 07:29:38 UTC
(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.
Comment 6 Hyunjun Ko 2015-07-02 08:44:33 UTC
Created attachment 306592 [details] [review]
ximagesrc: add meta transfrom fucntion
Comment 7 Sebastian Dröge (slomo) 2015-07-06 09:56:51 UTC
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 8 Sebastian Dröge (slomo) 2015-07-06 10:02:10 UTC
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
Comment 9 Hyunjun Ko 2015-07-06 11:19:26 UTC
Created attachment 306906 [details] [review]
ximagesrc: add meta transfrom fucntion

Following slomo's review
Comment 10 Hyunjun Ko 2015-07-06 11:19:59 UTC
Created attachment 306907 [details] [review]
mpegvideometa: add meta transform function

Following slomo's review.
Comment 11 Sebastian Dröge (slomo) 2015-07-06 21:15:15 UTC
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?
Comment 12 Hyunjun Ko 2015-07-07 02:25:40 UTC
Created attachment 306970 [details] [review]
mpegvideometa: add meta transform function
Comment 13 Hyunjun Ko 2015-07-07 02:26:11 UTC
Created attachment 306971 [details] [review]
meta: transform_func: return FALSE if not supported or failed
Comment 14 Hyunjun Ko 2015-07-07 02:26:49 UTC
Created attachment 306972 [details] [review]
video/audio meta: transform_func: return FALSE if not supported or failed
Comment 15 Hyunjun Ko 2015-07-07 02:27:21 UTC
Created attachment 306973 [details] [review]
glsyncmeta: transform func: return FALSE if not supported or failed
Comment 16 Sebastian Dröge (slomo) 2015-07-07 10:37:50 UTC
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 17 Sebastian Dröge (slomo) 2015-07-07 10:44:03 UTC
Comment on attachment 306970 [details] [review]
mpegvideometa: add meta transform function

Nevermind, I fixed that now ;)
Comment 18 Sebastian Dröge (slomo) 2015-07-07 10:46:10 UTC
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.
Comment 19 Hyunjun Ko 2015-07-07 12:49:55 UTC
(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 :)
Comment 20 Hyunjun Ko 2015-07-07 13:31:51 UTC
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.
Comment 21 Sebastian Dröge (slomo) 2015-07-07 13:41:49 UTC
Thanks, all pushed :) Now only the protectionmeta is missing?
Comment 22 Hyunjun Ko 2015-07-07 13:44:55 UTC
(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.