GNOME Bugzilla – Bug 797006
vaapidec: Requests a GstForceKeyUnit in case of parse/decode error.
Last modified: 2018-09-03 14:53:23 UTC
Created attachment 373421 [details] [review] Requests GstForceKeyUnit to the upstream in case of parse/decode error. This is a follow up of issue: https://bugzilla.gnome.org/show_bug.cgi?id=796863 Requests GstForceKeyUnit to the upstream in case of parse/decode error. In the case of decode error, there is a change that affects other cases. The "GstFlowret value was never initialized in some condition and so it was undefined. I saw by experience that it was 0 (GST_FLOW_OK) but this is a random behavior and tied to the specific compiler/cpu/kernel and other factors. I added a default value that it makes sense for my understanding of this codebase. But I'm probably wrong. At least I think it is better because it is a defined behavior. I saw that there are other functions that have this issue like gst_vaapidecode_push_all_decoded_frames where "ret" is returned in the default switch with the un-initialized value.
Review of attachment 373421 [details] [review]: Can you split the uninitialized value fix in its own patch please. Then we can add the correct bug ref in which that regression happened. Then you have break twice now.
Sorry, I'm not 100% sure what you prefer. Do you want that I split the patch and open a different bug for uninitialized or is it enough to split the patch in two separate patches but in this bug?
I guess it is enough to split the patch and post both here, in the same bug.
Yes that's fine, we will update the bug uri upon merge.
Created attachment 373446 [details] [review] Patch for requesting key unit in case of error
Created attachment 373447 [details] [review] Patch for requesting key unit in case of error
Created attachment 373448 [details] [review] Requests GstForceKeyUnit to the upstream in case of parse/decode error.
Created attachment 373449 [details] [review] Initializing return value.
Sorry for the noise, multiple submit of the patch file. This is a split and simplified version of the original patch.
Review of attachment 373448 [details] [review]: ::: gst/vaapi/gstvaapidecode.c @@ +1186,1 @@ /* just keep parsing, the decoder should have flushed the broken unit */ above this there a GST_ERROR ("parse error %d", status); Change it to GST_WARNING(... Though, I'm not sure if we should ditch the rest of frames until the key unit, or just drop the broken one.
Ah, another thing! Could you make, the patch for the initializing return value, the first one, and the key unit the second one. This is because the initializing issue is a bug that have to be backported into other branches, so making it independent of the new feature, will make the backport easier.
Yes, for sure. Thanks
*** Bug 796099 has been marked as a duplicate of this bug. ***
Created attachment 373533 [details] [review] Initializing return value.
Created attachment 373534 [details] [review] Requests GstForceKeyUnit to the upstream in case of parse/decode error.
Addressed comments, rebase on current master. Thanks for the feedback.
Comment on attachment 373533 [details] [review] Initializing return value. Commited the first patch with the bug fix, but I changed it a bit because ret is used by GST_VIDEO_DECODER_ERROR, so the returna value had to be defined before.
Comment on attachment 373533 [details] [review] [review] Initializing return value. Also pushed in branch 1.14 * 947a3204 (origin/1.14) vaapidecode: sets return value in failure case.