GNOME Bugzilla – Bug 724236
omx: Enhancements/cleanup for decoder/encoder frame handling
Last modified: 2014-07-23 08:24:13 UTC
Attached to this tracker are three patches which cleanup and enhance the frame handling code in the decoder and encoder. gstomxvideodec: remove dead code Just removes dead code from the omx decoder handling in preparation of the next two patches. gstomxvideodec: simplify _find_nearest_frame Drop the buffer identification map and just use the timestamp directly. This also fixes a (theoretical) race condition where the decoder returns the buffer faster than it gets added to the map. gstomxvideoenc: simplify _find_nearest_frame Does the same as the previous patch, but this time for the encoder. Please review, Christian.
Created attachment 268907 [details] [review] First patch.
Created attachment 268908 [details] [review] Second patch.
Created attachment 268909 [details] [review] Third patch.
Ping! Any news on this? Can we commit it?
I didn't get to review this yet but conceptionally this should be good to go :) Thanks for the patches. Are you also planning to submit the others ones you sent to the mailing list?
(In reply to comment #5) > Are you also planning to submit the others ones you sent to the mailing list? Yes, but while the first three where cleanups the rest is more like a RFC. I hope to create a general purpose implementation for OpenMAX tunneling. So that we can remove all those special RPI handling and use a proper OMX filter implementation instead.
Yeah but can you put that nonetheless in a bug? Ideally with a high level description of your idea how to implement tunneling properly in GStreamer without violating too many GStreamer concepts :) We can then discuss it there and try to find a good solution that works. Tunneling was on my list since some time too, so thanks for working on this :)
Btw, it would be great if you can give some more information about the hardware you're running this on and ideally if you could also provide a configuration file for it in the end that we could put next to the one for the RPi :)
Review of attachment 268908 [details] [review]: ::: omx/gstomxvideodec.c @@ +1017,3 @@ for (l = frames; l; l = l->next) { GstVideoCodecFrame *tmp = l->data; + GstClockTimeDiff diff = ABS (GST_CLOCK_DIFF (timestamp, tmp->pts)); If timestamp is GST_CLOCK_TIME_NONE this will give wrong results
Review of attachment 268907 [details] [review]: This one is good to go, please also check if the same changes you make are relevant for the audio encoder too Will push this here together with the other patches when they're ready.
Review of attachment 268909 [details] [review]: ::: omx/gstomxvideoenc.c @@ +560,3 @@ for (l = frames; l; l = l->next) { GstVideoCodecFrame *tmp = l->data; + GstClockTimeDiff diff = ABS (GST_CLOCK_DIFF (timestamp, tmp->pts)); Same as in the decoder
Created attachment 270798 [details] [review] Second patch v2
Created attachment 270799 [details] [review] Third patch v2
(In reply to comment #9) > If timestamp is GST_CLOCK_TIME_NONE this will give wrong results I've update the second and third patch with new versions. When the buffer timestamp is zero we now use zero for the gst timestamp as well. Please review again.
(In reply to comment #7) > Yeah but can you put that nonetheless in a bug? Ideally with a high level > description of your idea how to implement tunneling properly in GStreamer > without violating too many GStreamer concepts :) We can then discuss it there > and try to find a good solution that works. Tunneling was on my list since some > time too, so thanks for working on this :) I really plan to do so, but so far I have only a working prototype for video decoding tunneled to video encoding. I think the next step for me is to implement a filter component and when that's working try to get tunneling working with it in a more general fashion.
(In reply to comment #8) > Btw, it would be great if you can give some more information about the hardware > you're running this on and ideally if you could also provide a configuration > file for it in the end that we could put next to the one for the RPi :) Well, it's normal AMD gfx hardware we are working on :) http://www.phoronix.com/scan.php?page=news_item&px=MTU5NDc http://www.phoronix.com/scan.php?page=news_item&px=MTU5MTc The OpenMAX state tracker in mesa is still a bit to beta to use for production environment, but I'm working on it. What we basically want to do is expose UVD and VCE (video decoding and encoding) using OpenMAX.
Yeah that's the right change, I just noticed that I forgot to say what should happen with 0 timestamps :) Always using the oldest pending frame for them should be the best approach. I'm a bit worried that we might leak frames (already before your changes though), if the OpenMAX component decides to not give us one output frame per input frame. But then this really needs to be fixed in the component IMHO. commit 85db124673ca200dca00bde9760e7e30d2d495c4 Author: Christian König <christian.koenig@amd.com> Date: Thu Sep 5 03:41:10 2013 -0600 omxvideoenc: simplify _find_nearest_frame Just the same as we did with the decoder. Also give the function a gst_omx_video_enc prefix to distinct it from the decoder function. https://bugzilla.gnome.org/show_bug.cgi?id=724236 commit 2cfe70ed5d09af708a947321dba23f9b3e9de9c8 Author: Christian König <christian.koenig@amd.com> Date: Thu Sep 5 02:23:39 2013 -0600 omxvideodec: simplify _find_nearest_frame No need to make it more complicated and error prone than necessary. Also give the function a gst_omx_video_dec prefix to distinct it from the encoder function. https://bugzilla.gnome.org/show_bug.cgi?id=724236 commit bf0d2614c37491e145c03687691049b92838ea74 Author: Christian König <christian.koenig@amd.com> Date: Thu Sep 5 02:05:52 2013 -0600 omxvideodec: remove dead code This code doesn't seems to be used for quite a while, remove it before it starts to rot. https://bugzilla.gnome.org/show_bug.cgi?id=724236