After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 775948 - omxvideodec: Support for egl_render on RPi breaks dynamic resolution changes
omxvideodec: Support for egl_render on RPi breaks dynamic resolution changes
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-omx
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-12-11 12:42 UTC by minfrin
Modified: 2018-11-03 13:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fall back to original image format if EGL cannot be negotiated. (4.06 KB, patch)
2016-12-18 20:53 UTC, minfrin
needs-work Details | Review

Description minfrin 2016-12-11 12:42:32 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?
Comment 1 Sebastian Dröge (slomo) 2016-12-12 07:51:50 UTC
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.
Comment 2 minfrin 2016-12-18 20:53:50 UTC
Created attachment 342183 [details] [review]
Fall back to original image format if EGL cannot be  negotiated.
Comment 3 Sebastian Dröge (slomo) 2016-12-19 08:56:06 UTC
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?
Comment 4 minfrin 2017-01-01 21:20:41 UTC
> 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.
Comment 5 minfrin 2017-02-12 20:57:44 UTC
This patch also fixed the problem in https://bugzilla.gnome.org/show_bug.cgi?id=767801.
Comment 6 Julien Isorce 2017-05-22 10:08:47 UTC
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 7 Julien Isorce 2017-05-22 10:11:09 UTC
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.
Comment 8 Julien Isorce 2017-07-10 19:24:06 UTC
Should the title be "negotiation" instead of "resolution" ? (in comment 0 I can read "...and the dynamic negotiation works again...")
Comment 9 GStreamer system administrator 2018-11-03 13:00:44 UTC
-- 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.