GNOME Bugzilla – Bug 775948
omxvideodec: Support for egl_render on RPi breaks dynamic resolution changes
Last modified: 2018-11-03 13:00:44 UTC
When a dynamic resolution change occurs, gst_omx_video_dec_reconfigure_output_port() is called in order to ensure the change is negotiated correctly. In the original code path, the format is obtained from the GPU and maintained untouched: format = gst_omx_video_get_format_from_omx (port_def.format.video.eColorFormat); On the RPI specifically, support for egl_render kicks in and hard codes the format instead to RGBA: state = gst_video_decoder_set_output_state (GST_VIDEO_DECODER (self), GST_VIDEO_FORMAT_RGBA, port_def.format.video.nFrameWidth, port_def.format.video.nFrameHeight, self->input_state); This works fine if downstream supports RGBA, however omxh264enc does not and attempts to renegotiate I420. After renegotiating to I420, the above code hard codes the format back to RGBA and we hang. The workaround is to skip egl_render, and the dynamic negotiation works again. Question: why is it necessary to hard code RGBA here? Should we not just respect the original format?
For egl_render we have to force RGBA (it's the only thing it supports). If that fails and we need to renegotiate, we should let the component decide again though. That step seems to be missing.
Created attachment 342183 [details] [review] Fall back to original image format if EGL cannot be negotiated.
Review of attachment 342183 [details] [review]: ::: omx/gstomxvideodec.c @@ -967,3 @@ - - /* fallback: try to use EGLImage even if it is not in the caps feature */ - if (!gst_video_decoder_negotiate (GST_VIDEO_DECODER (self))) { If I'm not missing anything, this fallback case is missing now? Shouldn't it try negotiating with EGLImage/RGBA first, and only if that fails fall back to using no EGLImage as your patch does now?
> If I'm not missing anything, this fallback case is missing now? The fallback case isn't missing, no. Instead it properly falls back to the original code. > Shouldn't it try negotiating with EGLImage/RGBA first, and only > if that fails fall back to using no EGLImage as your patch does now? That's exactly what it does: - work out what the current format is, set that aside - try set/negotiate RGBA - if successful, follow the egl_render path; otherwise - if unsuccessful, follow the original code path as a fallback, restoring the original format Previously the code attempted to duplicate the original code, which in turn introduced the bug. That duplication has been removed and we revert back to the original code and behaviour.
This patch also fixed the problem in https://bugzilla.gnome.org/show_bug.cgi?id=767801.
Hi minfrin, thx a lot for your investigations. In comment 4 I think Sebastian was talking about the other fallback (see comment 3), not the software fallback. Could you split your patch in 2, one that just remove the 'accelerated' fallback case mentioned in comment 3 (I think you are right and it should be safe to remove it now, we left at the time we merged eglglessink into glimagesink few years ago, and when caps feature was not set depending on the gstgl versions used), and one patch that records the color format, because I think it is different problems. Then could you try to check if the 'color format' patch alone fixes the transcoding issue your reported in comment 0 ? If yes then the problem is that the dec_output_port has lost its initial color format after the 'tunnel' being disconnected (https://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c#n1094). So indeed we should record this color format at the beginning, like you did. Also it looks the issue tracker title does not match the comment 0, it should be "omxvideodec: Support for egl_render on RPi breaks negotiation with omxh264enc", something like that. (In reply to minfrin from comment #5) > This patch also fixed the problem in > https://bugzilla.gnome.org/show_bug.cgi?id=767801. It is not entirely exact, the real problem in that other tracker issue is the failure of the first 'accelerated' negotiation: https://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c#n956 With the first patch of the split mentioned above, it will still switch back to use system memory like reported in comment 0 https://bugzilla.gnome.org/show_bug.cgi?id=767801#c0 . That said I still think we need both of the 2 patches I mentioned above. First one for cleanup, second to fix transcoding. Thx!
Comment on attachment 342183 [details] [review] Fall back to original image format if EGL cannot be negotiated. Please address remarks in https://bugzilla.gnome.org/show_bug.cgi?id=775948#c6 . Thx in advance.
Should the title be "negotiation" instead of "resolution" ? (in comment 0 I can read "...and the dynamic negotiation works again...")
-- 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-omx/issues/8.