GNOME Bugzilla – Bug 754845
compilation fails if built in codec parsers are disabled
Last modified: 2015-09-17 13:49:36 UTC
I'm filing this bug on behalf of the reporter of http://lists.freedesktop.org/archives/gstreamer-devel/2015-September/054341.html The compilation of gstreamer-vaapi fails if --enable-builtin-codecparsers=no is set and gstreamer-1.5 is not used / linked. This is because mvc/hevc parsing was added in gstreamer-1.5 and the API calls are not fenced. So, either we check for gstreamer 1.5 if the built in codec parsers are not enabled, or we fence all the MVC/HEVC stuff verifying that we are using either gstreamer-1.5 or the built in codec parsers. I lean towards the first option, which is the simplest: in configure.ac, if codec-parsers are disabled, gstreamer-1.5 (at least) must be found. The other option is to modify a lot of code to fence all these.
(In reply to Víctor Manuel Jáquez Leal from comment #0) > I'm filing this bug on behalf of the reporter of > > http://lists.freedesktop.org/archives/gstreamer-devel/2015-September/054341. > html > > The compilation of gstreamer-vaapi fails if --enable-builtin-codecparsers=no > is set and gstreamer-1.5 is not used / linked. > > This is because mvc/hevc parsing was added in gstreamer-1.5 and the API > calls are not fenced. Which API ?? I can see some 1.5 protection code for mvc in gstvaapidecoder_h264.c and another patch in patches/videoparsers for disabling 3d in older version... Not sufficient ??? > > So, either we check for gstreamer 1.5 if the built in codec parsers are not > enabled, or we fence all the MVC/HEVC stuff verifying that we are using > either gstreamer-1.5 or the built in codec parsers. > > I lean towards the first option, which is the simplest: in configure.ac, if > codec-parsers are disabled, gstreamer-1.5 (at least) must be found. > The other option is to modify a lot of code to fence all these.
Aha, you are right! I would also go for option-1 ,even though it is bit awkward :)
Also I think a fix is needed even in against 1.5 as per http://lists.freedesktop.org/archives/libva/2015-September/003519.html..
Created attachment 311150 [details] [review] build: verify for H264 MVC and H265 SPS Currently the H264 and H265 parsers look for MVC and SPS respectively, and the required symbols for those were added in GStreamer 1.5 This patch verifies if the installed H264 and H265 parser have those symbols. If they do not have them, the built in parsers are enabled used. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311153 [details] [review] decoder: h264: initialize PPS's slice_group_id When the GstVaapiParserInfoH264 is allocated, the memory is not initialized, so it contains random data. When gst_h264_parser_parse_pps() fails, the PPS structure keeps slice_group_id pointer uninitialized, leading to a segmentation fault when the memory is freed. This patch prevents this by initializing the slice_group_id before the PPS parsing. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
(In reply to Víctor Manuel Jáquez Leal from comment #4) > Created attachment 311150 [details] [review] [review] > build: verify for H264 MVC and H265 SPS > > Currently the H264 and H265 parsers look for MVC and SPS respectively, and > the required symbols for those were added in GStreamer 1.5 > > This patch verifies if the installed H264 and H265 parser have those > symbols. If > they do not have them, the built in parsers are enabled used. > > Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> patch applied: build with --enable-builtin-codecparsers=no 1:4: gst-inspect-1.0 vaapiparse_h264 libgstvaapi_parse.so: undefined symbol: GST_PAD_SET_ACCEPT_TEMPLATE 1.2: gst-inspect-1.0 vaapiparse_h264: undefined symbol: gst_base_parse_merge_tags 1.6: gst-inspect-1.0 vaapiparse_h264: libgstvaapi_parse.so: undefined symbol: gst_h265_parser_free Am I messing-up something ? :)
> > patch applied: > build with --enable-builtin-codecparsers=no > > 1:4: gst-inspect-1.0 vaapiparse_h264 libgstvaapi_parse.so: undefined symbol: > GST_PAD_SET_ACCEPT_TEMPLATE > > > 1.2: gst-inspect-1.0 vaapiparse_h264: undefined symbol: > gst_base_parse_merge_tags > > > 1.6: gst-inspect-1.0 vaapiparse_h264: libgstvaapi_parse.so: undefined > symbol: gst_h265_parser_free > > Am I messing-up something ? :) Aha! ... I worked on this patch also disabling the video parsers: --enable-builtin-videoparser=no When the video parsers are enable, I see these errors too. Right now I'm working in GStreamer 1.4 GST-PAD-SET-ACCEPT-TEMPLATE was added in GStreamer 1.5 But patches/videoparsers/0002-h264parse-fix-build-with-older-GStreamer-1.x-stacks.patch enables it for GStreamer 1.3 http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstPad.html#GST-PAD-SET-ACCEPT-TEMPLATE:CAPS And we also need a similar patch for gsth265parse.c, which is not fenced. In the case of gst_base_parse_merge_tags(), it was added in GStreamer 1.5 and there is no patch to fence it. The same for gst_h265_parser_free(). Perhaps we shall only enable gsth265parse.c only when compiling against gstreamer 1.5 or above.
I got the same results too with sreenj. Additionally, I don't get the element vaapiparse_h264 in case builtin-codecparsers are not disabled. The shared object libgstvaapi_parse.so is produced but after make install, the so does not get copied to specified location. Moreover gst-inspect -b does not list any blacklisted files and vaapiparse_h264 is not shown by gst-inspect vaapi.
(In reply to Víctor Manuel Jáquez Leal from comment #7) > > Perhaps we shall only enable gsth265parse.c only when compiling against > gstreamer 1.5 or above. You mean, if --enable-builtin-codecparsers=no , right?
(In reply to Engin FIRAT from comment #8) > I got the same results too with sreenj. Additionally, I don't get the > element vaapiparse_h264 in case builtin-codecparsers are not disabled. The > shared object libgstvaapi_parse.so is produced but after make install, the > so does not get copied to specified location. Moreover gst-inspect -b does > not list any blacklisted files and vaapiparse_h264 is not shown by > gst-inspect vaapi. I think Your issue (http://lists.freedesktop.org/archives/gstreamer-devel/2015-September/054341.html) can be fixed by adding a conditional compilation in gst/vaapi/Makefilea.am
Created attachment 311301 [details] [review] build: verify for H264 MVC and H265 SPS Currently the H264 and H265 parsers look for MVC and SPS respectively, and the required symbols for those were added in GStreamer 1.5 This patch verifies if the installed H264 and H265 parser have those symbols. If they do not have them, the built in parsers are enabled used. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311302 [details] [review] decoder: h264: initialize PPS's slice_group_id When the GstVaapiParserInfoH264 is allocated, the memory is not initialized, so it contains random data. When gst_h264_parser_parse_pps() fails, the PPS structure keeps slice_group_id pointer uninitialized, leading to a segmentation fault when the memory is freed. This patch prevents this by initializing the slice_group_id before the PPS parsing. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311303 [details] [review] patches/videoparsers: h264parser: refresh patches In order to avoid the creation of .orig files and break the distcheck target. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311304 [details] [review] patches/videoparsers: h264parser: fix description and refresh Fix a typo in the patch description and refresh it in order to avoid the creation of .orig files and break the distcheck target. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311305 [details] [review] patches/videoparsers: h264parser: more API fences and refresh Add more API fences according with its version and refresh the patch. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311306 [details] [review] patches/videoparsers: h265parser: rename patch keeping number Refresh the patch and rename it in order to keep the patch number. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311307 [details] [review] patches/videoparsers: h265parser: more API fences Add more API fences according with its version and refresh the patch. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Review of attachment 311305 [details] [review]: There are some whitespace issues...
Review of attachment 311306 [details] [review]: There are some whitespace issues...
Review of attachment 311307 [details] [review]: There are some whitespace issues...
Review of attachment 311302 [details] [review]: The codecparser has the code for initializing the slice_group_id, but not at the beginning of parsing.. IMHO, It could be more correct to fix it in parser...
Review of attachment 311301 [details] [review]: Does it intended to fix the issue http://lists.freedesktop.org/archives/gstreamer-devel/2015-September/054341.html ? Not fixing the issue for me when building against 1.6 with --enable-builtin-codecparsers=no !
Review of attachment 311303 [details] [review]: lgtm
Review of attachment 311304 [details] [review]: lgtm,
(In reply to sreerenj from comment #21) > Review of attachment 311302 [details] [review] [review]: > > The codecparser has the code for initializing the slice_group_id, but not > at the beginning of parsing.. > IMHO, It could be more correct to fix it in parser... May be you can fix this in gstremaer-vaapi itself (as exactly in your patch) for now, and after 0.6.1 release fix it in codecparser so that you can avoid codecparser update for the bug fix release...
(In reply to sreerenj from comment #25) > (In reply to sreerenj from comment #21) > > Review of attachment 311302 [details] [review] [review] [review]: > > > > The codecparser has the code for initializing the slice_group_id, but not > > at the beginning of parsing.. > > IMHO, It could be more correct to fix it in parser... > > May be you can fix this in gstremaer-vaapi itself (as exactly in your patch) > for now, and after 0.6.1 release fix it in codecparser so that you can avoid > codecparser update for the bug fix release... I was thinking exactly that. But still I need to open a bug to codecparsers.
Created attachment 311395 [details] [review] decoder: h264: initialize PPS's slice_group_id When the GstVaapiParserInfoH264 is allocated, the memory is not initialized, so it contains random data. When gst_h264_parser_parse_pps() fails, the PPS structure keeps slice_group_id pointer uninitialized, leading to a segmentation fault when the memory is freed. This patch prevents this by initializing the slice_group_id before the PPS parsing. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311396 [details] [review] build: verify for H264 MVC and H265 SPS Currently the H264 and H265 parsers look for MVC and SPS respectively, and the required symbols for those were added in GStreamer 1.5 If we try to compile in GStreamer < 1.4, without enabling the builtin codec parsers, the compilation fails, because the lack of those symbols. This patch verifies if the installed H264 and H265 parsers have those symbols. If they do not, the specific built in codec parsers are enabled and used. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311397 [details] [review] build: link libgstvaapi_parse against codec parser GST_CODEC_PARSER_* variables are defined if builtin codec parsers are disabled when running configure. Right now, libgstcodecparsers links only to libgstvaapi, but libgstvaapi_parse need it if builtin codec parsers are disabled. This patch adds GST_CODEC_PARSER_* variables to libgstvaapi_parse compilation. If builtin codec parsers are enable, this variable is null, so it should work using libgstvaapi, as normal. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311398 [details] [review] patches/videoparsers: h264parser: refresh patches In order to avoid the creation of .orig files and break the distcheck target. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311399 [details] [review] patches/videoparsers: h264parser: fix description and refresh Fix a typo in the patch description and refresh it in order to avoid the creation of .orig files and break the distcheck target. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311400 [details] [review] patches/videoparsers: h264parser: more API fences and refresh Add more API fences according with its version and refresh the patch. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311401 [details] [review] patches/videoparsers: h265parser: rename patch keeping number Refresh the patch and rename it in order to keep the patch number. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 311402 [details] [review] patches/videoparsers: h265parser: more API fences Add more API fences according with its version and refresh the patch. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
I haven't tested myself (hope you done all necessary testing), but all patches LGTM ....
Attachment 311395 [details] pushed as f6ae00a - decoder: h264: initialize PPS's slice_group_id Attachment 311396 [details] pushed as ccc9ce7 - build: verify for H264 MVC and H265 SPS Attachment 311397 [details] pushed as 2aaafe9 - build: link libgstvaapi_parse against codec parser Attachment 311398 [details] pushed as ac43e1c - patches/videoparsers: h264parser: refresh patches Attachment 311399 [details] pushed as 0ec0dab - patches/videoparsers: h264parser: fix description and refresh Attachment 311400 [details] pushed as ad463c8 - patches/videoparsers: h264parser: more API fences and refresh Attachment 311401 [details] pushed as 5524af5 - patches/videoparsers: h265parser: rename patch keeping number Attachment 311402 [details] pushed as d20ae26 - patches/videoparsers: h265parser: more API fences
(In reply to sreerenj from comment #25) > (In reply to sreerenj from comment #21) > > Review of attachment 311302 [details] [review] [review] [review]: > > > > The codecparser has the code for initializing the slice_group_id, but not > > at the beginning of parsing.. > > IMHO, It could be more correct to fix it in parser... > > May be you can fix this in gstremaer-vaapi itself (as exactly in your patch) > for now, and after 0.6.1 release fix it in codecparser so that you can avoid > codecparser update for the bug fix release... I think that the h264 codec parser doesn't need to be fixed. The function that fills the structure GstH264PPS is gst_h264_parser_parse_pps () http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad-libs/html/gst-plugins-bad-libs-h264parser.html#gst-h264-parser-parse-pps And it receives, as parameter, the instance of GstH264PPS. It is not the obligation of the callee to initialize the structure, but the obligation of the caller, if I understand correctly.
(In reply to Víctor Manuel Jáquez Leal from comment #37) > (In reply to sreerenj from comment #25) > > (In reply to sreerenj from comment #21) > > > Review of attachment 311302 [details] [review] [review] [review] [review]: > > > > > > The codecparser has the code for initializing the slice_group_id, but not > > > at the beginning of parsing.. > > > IMHO, It could be more correct to fix it in parser... > > > > May be you can fix this in gstremaer-vaapi itself (as exactly in your patch) > > for now, and after 0.6.1 release fix it in codecparser so that you can avoid > > codecparser update for the bug fix release... > > I think that the h264 codec parser doesn't need to be fixed. The function > that fills the structure GstH264PPS is > > gst_h264_parser_parse_pps () > > http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad- > libs/html/gst-plugins-bad-libs-h264parser.html#gst-h264-parser-parse-pps > > And it receives, as parameter, the instance of GstH264PPS. It is not the > obligation of the callee to initialize the structure, but the obligation of > the caller, if I understand correctly. I wound't expect the user of API to fill the fields before calling the API.. :) Especially the number of fields in the struct is too much... We are creating the memory for slice_group_id() only when needed inside the API implementation. So I think is always better to move " /* set default values for fields that might not be present in the bitstream and have valid defaults */ pps->slice_group_id = NULL; " this part to the beginning of parser api.
(In reply to sreerenj from comment #38) > I wound't expect the user of API to fill the fields before calling the API.. > :) > Especially the number of fields in the struct is too much... > We are creating the memory for slice_group_id() only when needed inside the > API implementation. So I think is always better to move > " > /* set default values for fields that might not be present in the bitstream > and have valid defaults */ > pps->slice_group_id = NULL; > " > this part to the beginning of parser api. You have convinced me. Let's see if we can convince upstream: https://bugzilla.gnome.org/show_bug.cgi?id=755161