GNOME Bugzilla – Bug 749953
hevc decoder : missing includes
Last modified: 2015-05-28 07:37:10 UTC
Created attachment 304062 [details] [review] hevc decoder : missing includes build break as VASliceParameterBufferHEVC is undefined when hevc dec is detected.
Review of attachment 304062 [details] [review]: This is the preferred way... #ifdef HAVE_VA_VA_DEC_HEVC_H # include <va/va_dec_hevc.h> #endif
Review of attachment 304062 [details] [review]: Could you paste the build error? I cannot reproduce it :/ ::: gst-libs/gst/vaapi/gstvaapidecoder_h265.c @@ +29,3 @@ #include <string.h> #include <math.h> +#include <va/va.h> You should use #include "gstvaapicompat.h" @@ +30,3 @@ #include <math.h> +#include <va/va.h> +#include <va/va_dec_hevc.h> As Sree said, this should be guarded.
I disgress . Or should all the encoders/decoders in gst-libs/gst/vaapi/ do likewise (gstvaapiencoder_mpeg2.c or even the gstvaapiencoder_h265.c which I based upon). The va_dec_hevc header is required by this decoder implementation file or else VASliceParameterBufferHEVC is not defined. The whole file should not be processed if HAVE_VA_VA_DEC_HEVC_H is not defined .
Created attachment 304091 [details] build failure
Thanks for the log. (In reply to Alban Browaeys from comment #3) > I disgress . Or should all the encoders/decoders in gst-libs/gst/vaapi/ do > likewise (gstvaapiencoder_mpeg2.c or even the gstvaapiencoder_h265.c which I > based upon). Indeed. Perhaps we should fix the encoders. I haven't work on them. But you're patch is for a decoder, and if you look gstvaapidecoder_jpeg.c and gstvaapidecoder_vp8.c, they used. Besides, the reason of gstvaapicompat.h is to handle the differences between va-api version. > > The va_dec_hevc header is required by this decoder implementation file or > else VASliceParameterBufferHEVC is not defined. The whole file should not > be processed if HAVE_VA_VA_DEC_HEVC_H is not defined .
Created attachment 304108 [details] [review] hevc decoder : missing includes v2 Thanks Sree and Victor ! Here is a mix of both of your comment.
@Victor: Issue is not reproducible for you because, you are using libva from master branch where va_dec_hevc.h has included in va.h. For older versions you have to include it explicitly, actually even for latest released version 1.5.0 (VA-0.37).. :) @Alban Browaeys: The Makefile will compile the gstvaapidecoder_hevc.c only if there is support for HEVC and in that perspective any guard here is useless. But the purpose of this guard here is different. For VA >= 0.38 , the va.h will include va_dec_hevc.h indirectly. So you don't need to include the va_dec_hevc.h manually . In practice, right now HAVE_VA_VA_DEC_HEVC is still true. But in future we might explicitly ask for applications to include only va.h... So what I said in comment 1 is exactly what we need.
(In reply to Alban Browaeys from comment #6) > Created attachment 304108 [details] [review] [review] > hevc decoder : missing includes v2 > > Thanks Sree and Victor ! Here is a mix of both of your comment. Thanks for the patch, I will push it.
commit 3857d5fce1e38671cd1ff797deb250d95096793c Author: Alban Browaeys <prahal@yahoo.com> Date: Wed May 27 23:43:16 2015 +0300 HEVC: decode: add missing va_dec_hevc header Signed-off-by: Alban Browaeys <prahal@yahoo.com> Signed-off-by: Sreerenj Balachandran <sreerenj.balachandran@intel.com> https://bugzilla.gnome.org/show_bug.cgi?id=749953
(In reply to Alban Browaeys from comment #3) > I disgress . Or should all the encoders/decoders in gst-libs/gst/vaapi/ do > likewise (gstvaapiencoder_mpeg2.c or even the gstvaapiencoder_h265.c which I > based upon). > > The va_dec_hevc header is required by this decoder implementation file or > else VASliceParameterBufferHEVC is not defined. The whole file should not > be processed if HAVE_VA_VA_DEC_HEVC_H is not defined . You indeed don't need to add header guards in the file if that file is already not included during build if the designated codec is not available...