[Bug 778750] New: For vaapih264enc without h264parse, using the adaptive 8x8 DCT encoding tool breaks 720p 30 fps clips

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Bug ID: 778750
Summary: For vaapih264enc without h264parse, using the adaptive
8x8 DCT encoding tool breaks 720p 30 fps clips
Classification: Platform
Product: GStreamer
Version: 1.11.1
OS: Linux
Status: NEW
Severity: normal
Priority: Normal
Component: gstreamer-vaapi
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@intel.com
QA Contact: gstreamer-***@lists.freedesktop.org
CC: ***@gmail.com, ***@igalia.com
GNOME version: ---

The following command creates an invalid video clip:

gst-launch-1.0 -e videotestsrc num-buffers=60 !
video/x-raw,format=NV12,width=1280,height=720,framerate=30/1 ! vaapih264enc !
mp4mux ! filesink location=test.mp4

When playing with this command:
gst-play-1.0 -v --gst-debug=*:4,vaapi*:4 test.mp4

It throws this error:
0:00:00.073091622 1373 0x7ff9b0004850 WARN codecparsers_h264
gsth264parser.c:508:gst_h264_parse_vui_parameters: failed to read uint8, nbits:
1
0:00:00.073109398 1373 0x7ff9b0004850 WARN codecparsers_h264
gsth264parser.c:522:gst_h264_parse_vui_parameters: error parsing "VUI
Parameters"
0:00:00.073120611 1373 0x7ff9b0004850 WARN codecparsers_h264
gsth264parser.c:1776:gst_h264_parse_sps: error parsing "Sequence parameter set"
0:00:00.073133640 1373 0x7ff9b0004850 ERROR vaapidecode
gstvaapidecode.c:1094:gst_vaapidecode_parse_frame: parse error 8

Also running the clip through the Intel Video Pro Analyzer shows that there are
decoding issues with the Sequence Parameter Set and Picture Parameter Set.

This problem relates to the usage of the adaptive 8x8 DCT encoding tool and was
introduced when the flag for that feature was turned on:
4aec5bd encoder: h264: Use high profile by default

It is very specific to the 720p resolution and the 30 fps framerarate. When
testing VGA, 1080p and 4k, they all work. Also changing the framerate to e.g.
20 or 35 fps makes it work.

A workaround is to add an h264parse element to the capture command:
gst-launch-1.0 -e videotestsrc num-buffers=60 !
video/x-raw,format=NV12,width=1280,height=720,framerate=30/1 ! vaapih264enc !
h264parse ! mp4mux ! filesink location=test.mp4

libva info: VA-API version 0.39.4
libva info: va_getDriverName() returns 0
libva info: Trying to open /usr/lib/dri/i965_drv_video.so
libva info: Found init function __vaDriverInit_0_39
libva info: va_openDriver() returns 0
vainfo: VA-API version: 0.39 (libva 1.7.3)
vainfo: Driver version: Intel i965 driver for Intel(R) Broxton - 1.8.0.pre1
(1.7.3-287-g05d2d25)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

--- Comment #1 from sreerenj <***@gmail.com> ---
As I mentioned in the mail, "adding h264parse" is not a workaround. I always
prefer to have the h264parse after the encoder.

But it is good to check why the pipeline with out parser is failing.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Hyunjun Ko <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@igalia.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

--- Comment #2 from Hyunjun Ko <***@igalia.com> ---
Created attachment 347810
--> https://bugzilla.gnome.org/attachment.cgi?id=347810&action=edit
libs: encoder: h264: apply emulation prevention bytes

Applys EPB when providing codec data.
Otherwise produced h264 might be broken when decoder/parser,
considering EPB, starts processing.

-----

I refered to libav and x264enc.
Tested on gstreamer and vlc.
I think it needs to double-check.

@Soren Friis, Could you confirm this patch? again?:)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

--- Comment #3 from Soren Friis <***@intel.com> ---
Thanks. I can confirm that this patch fixes the problem that I have been
seeing.

I unfortunately cannot comment too much on the internal mechanics of this fix.
I.e. whether this is the correct way to fix it or not. You should probably get
someone else to review as well.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #347810|none |needs-work
status| |

--- Comment #4 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Comment on attachment 347810
--> https://bugzilla.gnome.org/attachment.cgi?id=347810
libs: encoder: h264: apply emulation prevention bytes

A couple comments about this patch:

1. I would document in the commit log what is the
emulation_prevention_three_bytes with the information in the H264 spec (7.3.1
NAL unit syntax and 7.4.1 NAL unit semantics)

2. This patch looks like a copy of what ffmpeg does here [1]. The license is
LGPL so we can reuse that code, but we have to honour the original author: Mark
Thompson <***@jkqxz.net> [2]

3. AFAIK H265 also uses this emulation prevention technique, so we should
refactor this code as a shared function.

4. My guts tell me that it could be a better way to add this emulation
prevention code, aligned with the gstvaapi code style.

1.
http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/vaapi_encode_h26x.c;h=d806f9b52b354fdf55070f3d0acc44c79948df94;hb=HEAD#l23
2.
http://git.videolan.org/?p=ffmpeg.git;a=commit;h=2c62fcdf5d617791a653d7957d449f75569eede0

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Hyunjun Ko <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #347810|needs-work |none
status| |
Attachment #347810|0 |1
is obsolete| |

--- Comment #5 from Hyunjun Ko <***@igalia.com> ---
Created attachment 348150
--> https://bugzilla.gnome.org/attachment.cgi?id=348150&action=edit
libs: encoder: h264/5: fix wrong return value

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

--- Comment #6 from Hyunjun Ko <***@igalia.com> ---
Created attachment 348151
--> https://bugzilla.gnome.org/attachment.cgi?id=348151&action=edit
libs: utils: h26x: creates vaapiutils_h26x

Since there are duplicated code in h264/265 encoder,
we could refactor these code to avoid duplicated code.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

--- Comment #7 from Hyunjun Ko <***@igalia.com> ---
Created attachment 348152
--> https://bugzilla.gnome.org/attachment.cgi?id=348152&action=edit
libs: utils: h26x: implements gst_vaapi_utils_h26x_write_nal_unit

Implements gst_vaapi_utils_h26x_write_nal_unit, which does
write nal unit's length and data to bitwriter.

Note that this applies EPB(Emulation Prevention Bytes).
Otherwise produced codec_data might be broken when decoder/parser
considering EPB, starts parsing.

See also Annex B of H264, which describes EPB.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348152|none |needs-work
status| |

--- Comment #8 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 348152:
--> (https://bugzilla.gnome.org/review?bug=778750&attachment=348152)

::: gst-libs/gst/vaapi/gstvaapiutils_h26x.c
@@ +78,3 @@
+ } else {
+ if ((src[sp] & ~3) == 0) {
+ // emulation_prevention_byte: 0x03

can you change this use the old C comment style? just for homogeneity.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348150|none |accepted-commit_now
status| |

--- Comment #9 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 348150:
--> (https://bugzilla.gnome.org/review?bug=778750&attachment=348150)

lgtm

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348151|none |needs-work
status| |

--- Comment #10 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 348151:
--> (https://bugzilla.gnome.org/review?bug=778750&attachment=348151)

::: gst-libs/gst/vaapi/gstvaapiutils_h26x.c
@@ +2,3 @@
+ * gstvaapiutils_h26x.c - H.26x related utilities
+ *
+ * Copyright (C) 2017 Intel Corporation

Add the Author tag, keeping the original authors of the code that you moved
here.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Hyunjun Ko <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348152|needs-work |none
status| |
Attachment #348152|0 |1
is obsolete| |

--- Comment #11 from Hyunjun Ko <***@igalia.com> ---
Created attachment 348157
--> https://bugzilla.gnome.org/attachment.cgi?id=348157&action=edit
libs: utils: h26x: implements gst_vaapi_utils_h26x_write_nal_unit

Implements gst_vaapi_utils_h26x_write_nal_unit, which does
write nal unit's length and data to bitwriter.

Note that this applies EPB(Emulation Prevention Bytes).
Otherwise produced codec_data might be broken when decoder/parser
considering EPB, starts parsing.

See also Annex B of H264, which describes EPB.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

--- Comment #12 from Hyunjun Ko <***@igalia.com> ---
I modified according to your sugestion in the third patch together.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348150|accepted-commit_now |committed
status| |

--- Comment #13 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Comment on attachment 348150
--> https://bugzilla.gnome.org/attachment.cgi?id=348150
libs: encoder: h264/5: fix wrong return value

Attachment 348150 pushed as 257cfb6 - libs: encoder: h264/5: fix wrong return
value

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348151|needs-work |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #348157|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

--- Comment #14 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
commit 9ed6ac1f763589dd3f77fdccfe6b3487c3a836ba
Author: Hyunjun Ko <***@igalia.com>
Date: Fri Mar 17 17:14:01 2017 +0900

libs: h26x: adds gst_vaapi_utils_h26x_write_nal_unit()

Implements gst_vaapi_utils_h26x_write_nal_unit(), which writes NAL
unit length and data to a bitwriter.

Note that this helper function applies EPB (Emulation Prevention
Bytes), since otherwise produced codec_data might be broken when
decoder/parser considering EPB, starts parsing.

See sections 7.3 and 7.4 of the H264 and H264 specifications, which
describes the emulation_prevention_three_byte.

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Signed-off-by: Víctor Manuel Jáquez Leal <***@intel.com>

commit 49b370ed600c713f0c309bf6091994393af2a6bd
Author: Hyunjun Ko <***@igalia.com>
Date: Fri Mar 17 16:49:41 2017 +0900

libs: utils: h26x: create vaapiutils_h26x

Since there is duplicated code in h264/265 encoder, we could
refactor it to avoid duplicated code.

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Signed-off-by: Víctor Manuel Jáquez Leal <***@intel.com>

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778750

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|git master |1.11.3