GNOME Bugzilla – Bug 793419
pnm: seg fault with non power of 4 strides in pnmenc
Last modified: 2018-02-14 08:20:33 UTC
Created attachment 368303 [details] Example video frame that does not countain padding I have found a segmentation fault with pnmenc caused due to the input frames not being handled as videobuffers. The stride of the input was calculated using the videoinfo collected from the caps. This defaults to the stride being a power of 4. If the input frames do not contain padding and do not have a width that is a multiplier of 4, the pnmenc would segment. I am attaching an example videoframe and a pipe to verify the patch: gst-launch-1.0 filesrc location= 1022x1024_rgb24_no_padding ! rawvideoparse format=rgb width=1022 height=1024 plane-strides='<3066>' ! pnmenc ! filesink location=dump.pnm
Created attachment 368304 [details] [review] Patch to fix segmentation fault
Review of attachment 368304 [details] [review]: ::: gst/pnm/gstpnmenc.c @@ +266,3 @@ o_rowstride = pnmenc->info.width; } + i_rowstride = GST_VIDEO_FRAME_PLANE_STRIDE (&in_frame, 0); How did this ever work? :) input_state is a GstVideoCodecState*, not a GstVideoFrame*!
Comment on attachment 368304 [details] [review] Patch to fix segmentation fault Does not apply to latest GIT master
Review of attachment 368304 [details] [review]: Does pnmenc advertise VideoMeta support ? If no, then we should also fix rawvideoparse. This patch remains a good idea, and then we should advertise VideoMeta. If yes, then this what we need kindred. I'll review more attentively soon
(In reply to Nicolas Dufresne (stormer) from comment #4) > Review of attachment 368304 [details] [review] [review]: > > Does pnmenc advertise VideoMeta support ? > > If no, then we should also fix rawvideoparse. This patch remains a good > idea, and then we should advertise VideoMeta. If yes, then this what we need > kindred. I'll review more attentively soon I am not sure how VideoMeta support is advertised, but the underlying element is a GstVideoEncoder. If the videometa support is not being advertised then there is a chance that other video encoders are broken as well. > Does not apply to latest GIT master This is odd. I tried cloning bad and applying the patch and it seems to apply just fine. Can you share what is breaking on your end so I can try to fix it?
(In reply to Dimitrios Katsaros from comment #5) > > (In reply to Nicolas Dufresne (stormer) from comment #4) > > Review of attachment 368304 [details] [review] [review] [review]: > > > > Does pnmenc advertise VideoMeta support ? > > > > If no, then we should also fix rawvideoparse. This patch remains a good > > idea, and then we should advertise VideoMeta. If yes, then this what we need > > kindred. I'll review more attentively soon > > I am not sure how VideoMeta support is advertised, but the underlying > element is a GstVideoEncoder. If the videometa support is not being > advertised then there is a chance that other video encoders are broken as > well. It's not being advertise atm. We need to implement propose_allocation() virtual, and add to the query meta API. But not a blocker from merging this change.
commit 3cf4a70dbc7af93eaf4198cfd20683ff61206405 Author: Dimitrios Katsaros <patcherwork@gmail.com> Date: Tue Feb 13 11:16:29 2018 +0100 pnm: Fixed segfault in pnmenc The pnmenc was not mapping the input buffers as video buffers. Because of this, the video frame stride was not being set based on frame but based on the caps, which make the assumption that the strides are a power of 4. For input that is not a power of 4, this would lead to a SIGSEGV. https://bugzilla.gnome.org/show_bug.cgi?id=793419