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 786173 - vaapi h264 decoder corrupted frame when there is resolution change
vaapi h264 decoder corrupted frame when there is resolution change
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
git master
Other Linux
: Normal normal
: 1.12.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-08-11 19:00 UTC by Matteo Valdina
Modified: 2017-09-28 13:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test application (32.37 KB, application/x-compressed-tar)
2017-08-11 19:00 UTC, Matteo Valdina
  Details
core dump for 1080p (3.16 MB, application/x-compressed-tar)
2017-08-14 16:36 UTC, Matteo Valdina
  Details
libs: decoder: h264: flush dpb if IDR is missing (2.14 KB, patch)
2017-08-16 02:45 UTC, Hyunjun Ko
rejected Details | Review
rewritten test app (in C) (6.44 KB, text/plain)
2017-08-23 19:07 UTC, Víctor Manuel Jáquez Leal
  Details
libs: decoder: h264: improve code-style (1.08 KB, patch)
2017-08-25 12:19 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapidecode: renegotiate always when sink caps change (1007 bytes, patch)
2017-08-25 12:19 UTC, Víctor Manuel Jáquez Leal
rejected Details | Review
libs: decoder: at update_caps() decode codec_data (1.58 KB, patch)
2017-08-25 12:19 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapidecode: call drain() when setting new format (977 bytes, patch)
2017-09-25 17:20 UTC, Víctor Manuel Jáquez Leal
none Details | Review
vaapiencode: flush pending frames before set format (2.02 KB, patch)
2017-09-25 17:20 UTC, Víctor Manuel Jáquez Leal
none Details | Review
vaapidecode: drain pending frames before set format (1.05 KB, patch)
2017-09-26 09:36 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapiencode: flush pending frames before set format (2.12 KB, patch)
2017-09-26 09:36 UTC, Víctor Manuel Jáquez Leal
committed Details | Review

Description Matteo Valdina 2017-08-11 19:00:17 UTC
Created attachment 357440 [details]
Test application

I'm writing a test application that encode and decode an H.264 stream.

The two pipeline (encoder pipeline and decoder pipeline) are connected via an udpsink and udpsrc on the loopback.

The problem is when the encoder changes resolution, the output on the decoder is corrupted. I tested with VLC and it is able to show correctly the stream.

From logs on the decoder side, I can see that the h264parse send the new caps to vaapidecodebin but the vaapidecode negotiate something like sink pad new resolution src pad old resolution.

In attachment my test source code.
Comment 1 Hyunjun Ko 2017-08-14 08:50:34 UTC
Please make it using byte-stream instead of avc when you want to change configuration in the middle of the stream.

In your case, put capsfilter "video/x-h264,stream-format=byte-stream" before h264parse element (after depay element) in your decoding pipeline.

I confirmed that your example is working after modification above.
Comment 2 Matteo Valdina 2017-08-14 15:48:09 UTC
Thanks,
It works.

 - Tested on my Kady lake GST 1.13.0 (libva 1.7.1) and it works without any issue.
 - Tested on Braswell GST 1.12.2 (libva 1.8.1)and it works but after awhile it core dump.

On the 1.12.2 it core dump when switching to 1920x1080. Please note that it starts at 1920x1080.

Do you know if from 1.12.2 and current master it was fixed something for buffer size like 1080p?
Comment 3 Víctor Manuel Jáquez Leal 2017-08-14 15:56:53 UTC
(In reply to Matteo Valdina from comment #2)
>  - Tested on Braswell GST 1.12.2 (libva 1.8.1)and it works but after awhile
> it core dump.
> 
> On the 1.12.2 it core dump when switching to 1920x1080. Please note that it
> starts at 1920x1080.

Please, upload the core dump to figure out what's the problem.
Comment 4 Matteo Valdina 2017-08-14 16:36:39 UTC
Created attachment 357564 [details]
core dump for 1080p

This is the core dump with 1.12.2
Comment 5 Hyunjun Ko 2017-08-15 04:24:09 UTC
(In reply to Matteo Valdina from comment #2)
> 
> On the 1.12.2 it core dump when switching to 1920x1080. Please note that it
> starts at 1920x1080.
> 
> Do you know if from 1.12.2 and current master it was fixed something for
> buffer size like 1080p?

I've seen the segfault problem, seems it should be fixed. I'm looking into this.
Comment 6 Hyunjun Ko 2017-08-15 08:08:54 UTC
This crash is caused by the case of exceeding the buffer size of udpsrc.
When 1080p is streamed, slice might be over 65536, which can be failed to parse entire slice IDR. 

@Matteo, please increase buffer-size of udpsrc.

@Victor, do we need to prevent this crash in decoder if something like this happens?
Comment 7 Víctor Manuel Jáquez Leal 2017-08-15 10:59:42 UTC
(In reply to Hyunjun Ko from comment #6)
> This crash is caused by the case of exceeding the buffer size of udpsrc.
> When 1080p is streamed, slice might be over 65536, which can be failed to
> parse entire slice IDR. 
> 
> @Matteo, please increase buffer-size of udpsrc.
> 
> @Victor, do we need to prevent this crash in decoder if something like this
> happens?

I'd say we shouldn't crash, but fail elegantly.
Comment 8 Matteo Valdina 2017-08-15 12:37:39 UTC
Hi @Hyunjun,
I did what you asked and it is not crashing on the 1.12.2, but I'm a little confused.

From my understanding, the udpsrc buffer-size is the size for the socket. I assume that it is translated to "setsockop(SO_RCV_BUFSIZE. buffer-size value)" or something similar.

Is this value used also for another purpose?
This value should only affect the buffer size if is too small will lead to a packet loss.
vaapih264dec is crashing due to an incomplete slice, like when there is a packet loss?

I plan to use this decoder for receiving a stream from a network so a packet loss could happen for multiple reasons.
Comment 9 Matteo Valdina 2017-08-15 14:52:53 UTC
Hi,

I noticed that after awhile that my resolution change test will hang. Checking dmesg logs shown these lines:

[ 8116.414921] [drm] stuck on bsd ring
[ 8116.424181] [drm] GPU HANG: ecode 8:1:0xcab2c083, in camera-source-0 [24680], reason: Ring hung, action: reset
[ 8116.424498] [drm:i915_set_reset_status [i915]] *ERROR* gpu hanging too fast, banning!
[ 8116.426752] drm/i915: Resetting chip after gpu hang

Should I report a bug to libva/intel-libva-driver?
Comment 10 Víctor Manuel Jáquez Leal 2017-08-15 15:44:38 UTC
@Matteo, usually, when a backtrace is requested, normally you just copy&paste the output of the bt command in gdb. There's no need of uploading the dump core, since it's big and almost useless, because you need the original setup to do a complete analysis of the memory. 

https://wiki.debian.org/HowToGetABacktrace
Comment 11 Víctor Manuel Jáquez Leal 2017-08-15 16:02:40 UTC
(In reply to Matteo Valdina from comment #8)
> Hi @Hyunjun,
> I did what you asked and it is not crashing on the 1.12.2, but I'm a little
> confused.
> 
> From my understanding, the udpsrc buffer-size is the size for the socket. I
> assume that it is translated to "setsockop(SO_RCV_BUFSIZE. buffer-size
> value)" or something similar.
> 
> Is this value used also for another purpose?
> This value should only affect the buffer size if is too small will lead to a
> packet loss.
> vaapih264dec is crashing due to an incomplete slice, like when there is a
> packet loss?
> 
> I plan to use this decoder for receiving a stream from a network so a packet
> loss could happen for multiple reasons.

That might a bug in udpsrc fixed in master, and that needs to be backported to 1.12
Comment 12 Hyunjun Ko 2017-08-16 00:45:52 UTC
(In reply to Matteo Valdina from comment #8)
> Hi @Hyunjun,
> I did what you asked and it is not crashing on the 1.12.2, but I'm a little
> confused.
> 
> From my understanding, the udpsrc buffer-size is the size for the socket. I
> assume that it is translated to "setsockop(SO_RCV_BUFSIZE. buffer-size
> value)" or something similar.
> 
> Is this value used also for another purpose?
> This value should only affect the buffer size if is too small will lead to a
> packet loss.
> vaapih264dec is crashing due to an incomplete slice, like when there is a
> packet loss?
> 
> I plan to use this decoder for receiving a stream from a network so a packet
> loss could happen for multiple reasons.

Crash should be fixed as Victor said. You can report if this kind of issue haapens. We're also trying to deal with streaming cases.

eg. #bug 777506
Comment 13 Hyunjun Ko 2017-08-16 02:45:20 UTC
Created attachment 357685 [details] [review]
libs: decoder: h264: flush dpb if IDR is missing

If configuration is changed during decoding, decoder requires IDR to reset
some state of decoder. But if it fails to get IDR, it could be crashed
due to trying to overwrite dpb list.

This might happen in case of streaming.
Comment 14 Víctor Manuel Jáquez Leal 2017-08-23 19:03:16 UTC
I got bitten by the GPU hang using gstreamer 1.12. I suspect a lot with that evil thread which changes the resolution.

I have rewritten the test app using a GSource for the change resolution (since you're using the GMainLoop). With it, there's no GPU hang as far as I have seen.
Comment 15 Víctor Manuel Jáquez Leal 2017-08-23 19:07:45 UTC
Created attachment 358262 [details]
rewritten test app (in C)

This  test app is written in C but that shouldn't be a problem or impose a big difference.
Comment 16 Víctor Manuel Jáquez Leal 2017-08-23 19:25:55 UTC
with 1.12

* we need to backport bug 754885 (and perhaps others)
* the pipeline blocks a while when changing to certain resolutions

with either master and 1.12 the corrupted frames remains when the resolution changes, except when it backs to the original resolution.
Comment 17 Víctor Manuel Jáquez Leal 2017-08-25 12:19:13 UTC
Created attachment 358392 [details] [review]
libs: decoder: h264: improve code-style
Comment 18 Víctor Manuel Jáquez Leal 2017-08-25 12:19:19 UTC
Created attachment 358393 [details] [review]
vaapidecode: renegotiate always when sink caps change
Comment 19 Víctor Manuel Jáquez Leal 2017-08-25 12:19:25 UTC
Created attachment 358394 [details] [review]
libs: decoder: at update_caps() decode codec_data

When updating the caps in decoder, if the caps has codec_data (avC
format), it has to be parsed to update the state of the decoder.
Comment 20 Víctor Manuel Jáquez Leal 2017-08-25 12:24:12 UTC
you can patch your test to use stream-format=byte-stream  before decoder, then the frames will have the information to decode them correctly (if they are supported by the level).

In the case of avc stream-format (the default), there's a bug when resetting the internal decoder. The posted patches aims to solve it. This bug is a regression of bug 781142.
Comment 21 Víctor Manuel Jáquez Leal 2017-08-25 12:31:55 UTC
Also, remove the tee in the server side. Until we fix bug 785054, tee might lead to problems.
Comment 22 Víctor Manuel Jáquez Leal 2017-08-28 16:40:54 UTC
Comment on attachment 358392 [details] [review]
libs: decoder: h264: improve code-style

commit d9c88f47
Comment 23 Matteo Valdina 2017-09-14 13:38:01 UTC
Thanks, I'll test your version.
Comment 24 Víctor Manuel Jáquez Leal 2017-09-14 15:00:28 UTC
Review of attachment 358393 [details] [review]:

the renegotiations shall be done only if the parser didn't fail after reconfiguring
Comment 25 Víctor Manuel Jáquez Leal 2017-09-14 15:28:12 UTC
Comment on attachment 358394 [details] [review]
libs: decoder: at update_caps() decode codec_data

Attachment 358394 [details] pushed as 2eb2b26 - libs: decoder: at update_caps() decode codec_data
Comment 26 Víctor Manuel Jáquez Leal 2017-09-14 16:12:37 UTC
Review of attachment 357685 [details] [review]:

I don't understand under what circumstances this dpb overwrite occur. I reproduced with the provided test app, but I'm not sure if the app does the correct thing.
Comment 27 Víctor Manuel Jáquez Leal 2017-09-25 15:03:12 UTC
Comment on attachment 357685 [details] [review]
libs: decoder: h264: flush dpb if IDR is missing

this should not happen, the decoder should flush, purge or drain the pending frames before changing caps
Comment 28 Víctor Manuel Jáquez Leal 2017-09-25 17:20:40 UTC
Created attachment 360367 [details] [review]
vaapidecode: call drain() when setting new format
Comment 29 Víctor Manuel Jáquez Leal 2017-09-25 17:20:46 UTC
Created attachment 360368 [details] [review]
vaapiencode: flush pending frames before set format
Comment 30 Víctor Manuel Jáquez Leal 2017-09-26 09:36:29 UTC
Created attachment 360414 [details] [review]
vaapidecode: drain pending frames before set format

Drain pending frames, if any, in the internal decoder before setting
the new negotiated format.
Comment 31 Víctor Manuel Jáquez Leal 2017-09-26 09:36:35 UTC
Created attachment 360415 [details] [review]
vaapiencode: flush pending frames before set format

Flush pending frames, if any, in the internal encorder, before setting
the new negotiated format.
Comment 32 Víctor Manuel Jáquez Leal 2017-09-26 09:41:01 UTC
Attachment 360414 [details] pushed as a4daa2a - vaapidecode: drain pending frames before set format
Attachment 360415 [details] pushed as 9f4a576 - vaapiencode: flush pending frames before set format
Comment 33 Víctor Manuel Jáquez Leal 2017-09-26 09:47:40 UTC
branch 1.12

* e5ef90f9 vaapiencode: flush pending frames before set format
* 6b98d6a0 vaapidecode: drain pending frames before set format
* a4c60fb2 libs: decoder: h264/h265: decode codec data only if opened
* 9cf26c81 libs: decoder: at update_caps() decode codec_data