GNOME Bugzilla – Bug 736252
gdkpixbufdec: packetized mode logic
Last modified: 2016-06-08 06:31:17 UTC
To check packetized mode the logic being used is /* packetised mode? *//* FIXME: shouln't this be fps_d != 0, since 0/1 * might be packetised mode but variable framerate */ if (filter->in_fps_n != 0) { I guess the logic is correct, since if numerator is 0, it will be considered as a single image. In that case FIXME should be removed right? And i am getting confused, with the logic used in pngdec element if (GST_VIDEO_INFO_FPS_N (info) != 1 && GST_VIDEO_INFO_FPS_D (info) != 1) gst_video_decoder_set_packetized (decoder, TRUE); Isn't this wrong? 1/1 framerate is also packetized mode right? Shouldn't the condition be, to check for numerator != 0, similar to gdkpixbufdec? if (GST_VIDEO_INFO_FPS_N (info) != 0 && GST_VIDEO_INFO_FPS_D (info) != 1) gst_video_decoder_set_packetized (decoder, TRUE);
(In reply to comment #0) > To check packetized mode the logic being used is > > /* packetised mode? *//* FIXME: shouln't this be fps_d != 0, since 0/1 > * might be packetised mode but variable framerate */ > if (filter->in_fps_n != 0) { > > I guess the logic is correct, since if numerator is 0, it will be considered as > a single image. In that case FIXME should be removed right? I'd leave the FIXME there, there doesn't seem to be a consensus on how to detect packetized input atm. Well, one could use the parsed boolean in caps but I guess this isn't everywhere yet. > > And i am getting confused, with the logic used in pngdec element > > if (GST_VIDEO_INFO_FPS_N (info) != 1 && GST_VIDEO_INFO_FPS_D (info) != 1) > gst_video_decoder_set_packetized (decoder, TRUE); > > Isn't this wrong? 1/1 framerate is also packetized mode right? > Shouldn't the condition be, to check for numerator != 0, similar to > gdkpixbufdec? Yes, this is wrong. Can you provide a fix? I'd keep the FPS_D != 1 check as this is comparing against the default value of 0/1, meaning that no framerate was provided in caps (likely). > > if (GST_VIDEO_INFO_FPS_N (info) != 0 && GST_VIDEO_INFO_FPS_D (info) != 1) > gst_video_decoder_set_packetized (decoder, TRUE);
Created attachment 285979 [details] [review] patch for pngdec
Created attachment 285980 [details] [review] patch for jpegdec
Created attachment 285981 [details] [review] patch for webpdec
0/1 is variable framerate, so this also doesn't make sense. To fix this properly we should add packetized/parsed fields to the caps everywhere. This would be a backwards compatible change as without the field the only effect would be that the decoders parse data that is already parsed.
For all these formats you can probably assume that if the input segment is in TIME that it's packetised, and if it's in BYTES, then it's not packetised, no?
Good point, that should work
Created attachment 286100 [details] [review] Modified as per input segement format Hi, Thanks for the review. I have modified pngdec as per the input segment format. Please review if this is proper, so that i can apply to other decoders. Regards, Vineeth
Comment on attachment 286100 [details] [review] Modified as per input segement format Seems correct, thanks! Please update the other patches too :) Also please fix your name and mail address in GIT and recreate this patch then.
Created attachment 286263 [details] [review] modified as per input segment format
Created attachment 286264 [details] [review] modified as per input segment format
Created attachment 286265 [details] [review] modified as per input segment format
Created attachment 286266 [details] [review] modified as per input segment format
Made the changes for all decoders. But the same cannot be applied to gdkpixbufdec right? should we rewrite gdkpixbufdec by using VideoDecoder base class instead of GstElement?
(In reply to comment #14) > Made the changes for all decoders. > > But the same cannot be applied to gdkpixbufdec right? Why not? It can also do that logic based on the input segment format > should we rewrite gdkpixbufdec by using VideoDecoder base class instead of > GstElement? Yes, that would be good :)
Thanks for the review. sorry about the mistake in jpegdec :) i will raise a new bug for gdkpixbufdec porting to VideoDecoder base class
Porting gdkpixbufdec to the base class is independent of fixing this though. You can just check the segment format as with the others :)
Created attachment 286283 [details] [review] gdkpixbufdec packetized logic Please review the changes for modifying packetized mode logic in gdkpixbufdec
commit 89b9313e200b8de8d34241a1119340af84f518c1 Author: Vineeth T M <vineeth.tm@samsung.com> Date: Tue Sep 16 16:46:07 2014 +0530 gdkpixbufdec: modify wrong packetized mode logic packetized mode is being set when framerate is being set which is not correct. Changing the same by checking the input segement format. If input segment is in TIME it is Packetized, and if it is in BYTES it is not. https://bugzilla.gnome.org/show_bug.cgi?id=736252
All those patches are wrong, because the CAPS arrives before the segment. The test dec->input_segment.format will always be undefined until you re-negotiate. Combine with the poor quality of the parser (which call memory_map/unmap multiple time for the same buffer), this cause a massive performance regression with v4l2src/dmabuf jpegs.
Created attachment 329230 [details] [review] jpegdec: Wait for segment event before checking it The heuristic to choose between packetise or not was change to use the segment format. The problem is that this change is reading the segment during the caps event handling. The segment event will only be sent after. That prevented the decoder to go in packetize mode, and avoid useless parsing.
We need to fix for all the others now (should be copy paste exercise).
Nicolas, are you also doing the others? Should get this into 1.8.2 really :) Thanks for spotting this (how did you?)
Yes, I'll do that today. I notice a huge performance regression with: v4l2src io-mode=dmabuf ! jpegdec ! fakesink It was caused by the parser in there being badly implemented. For each buffer, it will map/unmap hundreds of time (through calling the scan function), which endup using a full CPU (since map/unmap in this case actually map the dmabuf). But then Olivier made me realized it shouldn't be parsing for this case, since we know that the stream from V4L2 is packetized already. That's how we found it. I'll file a bug for the parser later.
I can only finish this one tomorrow morning, though if needed go ahead, it's simple enough.
Created attachment 329351 [details] [review] pngdec: Wait for segment event before checking it The heuristic to choose between packetise or not was changed to use the segment format. The problem is that this change is reading the segment during the caps event handling. The segment event will only be sent after. That prevented the decoder to go in packetize mode, and avoid useless parsing.
Created attachment 329352 [details] [review] vmncdec: Wait for segment event before checking it The heuristic to choose between packetise or not was changed to use the segment format. The problem is that this change is reading the segment during the caps event handling. The segment event will only be sent after. That prevented the decoder to go in packetize mode, and avoid useless parsing.
Created attachment 329359 [details] [review] webpdec: Wait for segment event before checking it The heuristic to choose between packetise or not was changed to use the segment format. The problem is that this change is reading the segment during the caps event handling. The segment event will only be sent after. That prevented the decoder to go in packetize mode, and avoid useless parsing.
All tested, except for vmnc, for which I don't have an encoder or a test file.
Attachment 329230 [details] pushed as a6008fd - jpegdec: Wait for segment event before checking it Attachment 329351 [details] pushed as 7af9271 - pngdec: Wait for segment event before checking it
Attachment 329352 [details] pushed as 50537e2 - vmncdec: Wait for segment event before checking it Attachment 329359 [details] pushed as d33352e - webpdec: Wait for segment event before checking it
Branch 1.8 Bad WebP Commit: 55297c7df662c8316312110104ee806742226db2 VMNC Commit: c511e0c223817de89a85b5614c8c58c83ede2d00 Good PNG Commit: c0fec05e5c95c9391278695f0b5ebbef81f1f009 JPG Commit: 81060df8baca29084c46431441e032a585062e6a
Thanks!