GNOME Bugzilla – Bug 712343
riff: Make use of the various helper to handle video formats
Last modified: 2018-11-03 11:26:28 UTC
That means the gst_pb_utils_get_codec_description function to get codec description and us GstVideoInfo to construct caps for raw videos.
Created attachment 259844 [details] [review] riff: Make use of the various helper to handle video formats
Review of attachment 259844 [details] [review]: Looks good in general, except for: ::: gst-libs/gst/riff/riff-media.c @@ +82,3 @@ + break; + case 16: + format = GST_VIDEO_FORMAT_RGB16; Are 15 and 16 valid here? I don't see them in the old code. @@ +85,3 @@ + break; + case 24: + format = GST_VIDEO_FORMAT_RGB; This was BGR, not RGB @@ +88,3 @@ + break; + case 32: + format = GST_VIDEO_FORMAT_ARGB; This should be BGRx, not ARGV @@ -900,3 @@ caps = gst_caps_new_empty_simple ("video/x-fraps"); - if (codec_name) - *codec_name = g_strdup ("Fraps video"); This isn't in pbutils, neither is x-xsub, we shoudl go over the entire list first before we remove them. Either we should add it there or leave this here.
Thibault?
Created attachment 289662 [details] [review] > ::: gst-libs/gst/riff/riff-media.c > @@ +82,3 @@ > + break; > + case 16: > + format = GST_VIDEO_FORMAT_RGB16; > > Are 15 and 16 valid here? I don't see them in the old code. Indeed it was not in the old code, removed > @@ +85,3 @@ > + break; > + case 24: > + format = GST_VIDEO_FORMAT_RGB; > > This was BGR, not RGB My mistake, FIXED > @@ +88,3 @@ > + break; > + case 32: > + format = GST_VIDEO_FORMAT_ARGB; > > This should be BGRx, not ARGV My mistake, FIXED > @@ -900,3 @@ > caps = gst_caps_new_empty_simple ("video/x-fraps"); > - if (codec_name) > - *codec_name = g_strdup ("Fraps video"); > This isn't in pbutils, neither is x-xsub, we shoudl go over the entire list > first before we remove them. Either we should add it there or leave this here. I added them in pbutils and went over the whole list adding the other missing ones
Created attachment 290802 [details] [review] riff: Make use of the various helper to handle video formats That means the gst_pb_utils_get_codec_description function to get codec description and us GstVideoInfo to construct caps for raw videos. + Add missing codec description in pbutils to avoid regressions
People are going to make fun of us for wavparse depending on libgstvideo ;)
(In reply to comment #6) > People are going to make fun of us for wavparse depending on libgstvideo ;) Is there something we can do about that? I any user of libriff already depends on libgstaudio even if totaly unrelated to their use.
What should we do about that?
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/gst-plugins-base/issues/93.