GNOME Bugzilla – Bug 756563
pnmdec: causes multifilesrc to return internal data flow error (caused by a gst_pad_push() error)
Last modified: 2015-10-28 07:45:33 UTC
When trying to use multifilesrc with the pnmdec I get an "Internal data flow error" after the first image Record a stream: gst-launch-1.0 videotestsrc num-buffers=10 ! pnmenc ! multifilesink Play the stream: gst-launch-1.0 multifilesrc ! pnmdec ! fakesink ERROR: from element /GstPipeline:pipeline0/GstMultiFileSrc:multifilesrc0: Internal data flow error. Additional debug info: /var/lib/jenkins/jobs/qt5022-cesium/workspace/build/tmp/work/bobcat_64-poky-linux/gstreamer1.0/git-r0/git/libs/gst/base/gstbasesrc.c(2949): gst_base_src_loop (): /GstPipeline:pipeline0/GstMultiFileSrc:multifilesrc0: gst-launch-1.0 multifilesrc ! pnmdec ! fakesink Setting --gst-debug=*src*:5 will show that it is a gst_pad_push() error: ... 0:00:00.027635899 788 0x611400 DEBUG multifilesrc gstmultifilesrc.c:381:gst_multi_file_src_get_filename: 0 0:00:00.027793246 788 0x611400 DEBUG multifilesrc gstmultifilesrc.c:414:gst_multi_file_src_create:<multifilesrc0> reading from file "00000". 0:00:00.028805526 788 0x611400 DEBUG multifilesrc gstmultifilesrc.c:456:gst_multi_file_src_create:<multifilesrc0> read file "00000". ... 0:00:00.032062416 788 0x690490 DEBUG basesrc gstbasesrc.c:3810:gst_base_src_change_state:<multifilesrc0> PAUSED->PLAYING 0:00:00.032105420 788 0x611400 DEBUG basesrc gstbasesrc.c:2356:gst_base_src_update_length:<multifilesrc0> reading offset 230415, length 4096, size -1, segment.stop -1, maxsize -1 0:00:00.032194276 788 0x611400 DEBUG basesrc gstbasesrc.c:2457:gst_base_src_get_range:<multifilesrc0> calling create offset 230415 length 4096, time 0 0:00:00.032224544 788 0x611400 DEBUG multifilesrc gstmultifilesrc.c:381:gst_multi_file_src_get_filename: 1 0:00:00.032249722 788 0x611400 DEBUG multifilesrc gstmultifilesrc.c:414:gst_multi_file_src_create:<multifilesrc0> reading from file "00001". New clock: GstSystemClock 0:00:00.032800276 788 0x611400 DEBUG multifilesrc gstmultifilesrc.c:456:gst_multi_file_src_create:<multifilesrc0> read file "00001". 0:00:00.033002571 788 0x611400 DEBUG basesrc gstbasesrc.c:2317:gst_base_src_do_sync:<multifilesrc0> no sync needed 0:00:00.033047402 788 0x611400 DEBUG basesrc gstbasesrc.c:2521:gst_base_src_get_range:<multifilesrc0> buffer ok 0:00:00.033116960 788 0x611400 INFO basesrc gstbasesrc.c:2857:gst_base_src_loop:<multifilesrc0> pausing after gst_pad_push() = error 0:00:00.033163011 788 0x611400 DEBUG basesrc gstbasesrc.c:2900:gst_base_src_loop:<multifilesrc0> pausing task, reason error 0:00:00.033301061 788 0x611400 WARN basesrc gstbasesrc.c:2949:gst_base_src_loop:<multifilesrc0> error: Internal data flow error. 0:00:00.033376359 788 0x611400 WARN basesrc gstbasesrc.c:2949:gst_base_src_loop:<multifilesrc0> error: streaming task paused, reason error (-5) ERROR: from element /GstPipeline:pipeline0/GstMultiFileSrc:multifilesrc0: Internal data flow error. Note that if jpeg/png (even raw) formats are used it works, therefore I assume the problem must be in the pnmdec... gst-launch-1.0 videotestsrc num-buffers=10 ! jpegenc ! multifilesink gst-launch-1.0 multifilesrc ! jpegdec ! fakesink or gst-launch-1.0 videotestsrc num-buffers=10 ! pngenc ! multifilesink gst-launch-1.0 multifilesrc ! pngdec ! fakesink
Created attachment 313520 [details] [review] properly reset parsing state at flush This sorts out the incomplete parser status reset at flush, solves the problem but uncovers others, namely, the element not forwarding the input state properly, see second patch in this series.
Created attachment 313521 [details] [review] properly propagate input state propagates input state so caps="...framerate=" works with ie ! videorate
Review of attachment 313520 [details] [review]: ::: gst/pnm/gstpnmdec.c @@ +204,3 @@ r = gst_video_decoder_allocate_output_frame (decoder, frame); if (r != GST_FLOW_OK) { + r = gst_video_decoder_drop_frame (GST_VIDEO_DECODER (s), frame); ^ This small change from my debugging slip through the cracks but according to the docs on the baseclass should be correct?
None of the commit messages actually mention what these patches fix, just that they're doing something "properly" now :)
Created attachment 313547 [details] [review] propagate input state after parsing
Created attachment 313548 [details] [review] completely reset parsing state at flush The new commit msgs should be more descriptive. Thanks for taking a look.
Review of attachment 313548 [details] [review]: ::: gst/pnm/gstpnmdec.c @@ +99,2 @@ s->size = 0; s->current_size = 0; Or just memset() it all to 0 just to be sure and future-proof? Or add a reset() function that resets all these and is also called by start() :)
Created attachment 313562 [details] [review] completely reset parsing state at flush (In reply to Sebastian Dröge (slomo) from comment #7) > Review of attachment 313548 [details] [review] [review]: > [...] > Or just memset() it all to 0 just to be sure and future-proof? Or add a > reset() function that resets all these and is also called by start() :) Hey Sebastian. Thanks for taking a look. Are you referring to the s->mngr parser values? I'm not sure resetting the entire codec state in 's' is correct here & it's not needed to resolve this issue. New patch attached. I have a couple of other minor changes for pnmdec after working on it for a bit. I will include your reset() idea with them.
Created attachment 313571 [details] [review] complete reset parsing state at flush Separated the unrelated frame drop change at handle() failure
Created attachment 313572 [details] [review] drop frame in case of _handle_frame() failure
Comment on attachment 313572 [details] [review] drop frame in case of _handle_frame() failure Here you would override the non-OK return value with potentially OK or something else.
Created attachment 313632 [details] [review] drop frame in case of handle failure You are right, actually did it on purpose but now I understood it didn't make sense. keeping original return value now. Thanks again for taking a look.
Review of attachment 313547 [details] [review]: commit ec5648763dd00b09ae2522372d453417ae8403f0 Author: Reynaldo H. Verdejo Pinochet <reynaldo@osg.samsung.com> Date: Sat Oct 17 01:51:24 2015 -0700 pnmdec: propagate input state after parsing Store and copy input state fields when setting the output state of the decoder. Avoids problems like the framerate set by an upstream element being ignored Related to: https://bugzilla.gnome.org/show_bug.cgi?id=756563
Review of attachment 313571 [details] [review]: commit 4b5e2f68a3767ddd66681c7a20c05bb2829c3234 Author: Reynaldo H. Verdejo Pinochet <reynaldo@osg.samsung.com> Date: Fri Oct 16 20:45:42 2015 -0700 pnmdec: completely reset parsing state at flush Makes sure the mngr struct reflects a clean state for the next frame, avoiding failures like: https://bugzilla.gnome.org/show_bug.cgi?id=756563
Review of attachment 313632 [details] [review]: commit 801c27689e76822bc558a513f7ffae87af001406 Author: Reynaldo H. Verdejo Pinochet <reynaldo@osg.samsung.com> Date: Sat Oct 17 12:48:11 2015 -0700 pnmdec: drop frame in case of _handle() failure Allows baseclass to handle it from there Related to: https://bugzilla.gnome.org/show_bug.cgi?id=756563
Should this go into 1.6.1?
(In reply to Sebastian Dröge (slomo) from comment #16) > Should this go into 1.6.1? Hey, sorry for the late reply. Yes, it should be safe. I saw you merged 78c22c3cbd492ab already but the other two are the ones actually fixing the problem.
Can you merge them?
(In reply to Sebastian Dröge (slomo) from comment #18) > Can you merge them? Double checked, they were all merged by you last Tuesday ;)