platform/upstream/gstreamer.git
6 years agolibs: display: remove display_type member
Víctor Manuel Jáquez Leal [Thu, 28 Sep 2017 15:35:01 +0000 (17:35 +0200)]
libs: display: remove display_type member

It is not used any more since GstVaapiDisplay was ported as a
GstObject-based. This information is part of the class information.

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

6 years agolibs: display: remove parent member
Víctor Manuel Jáquez Leal [Thu, 28 Sep 2017 14:12:23 +0000 (16:12 +0200)]
libs: display: remove parent member

Parent was a crumb left from display cache.

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

6 years agovaapi: Also register vaapipostproc without VPP support
Sebastian Dröge [Tue, 3 Oct 2017 11:06:33 +0000 (13:06 +0200)]
vaapi: Also register vaapipostproc without VPP support

It can still do simple deinterlacing then.

6 years agovaapipostproc: Allow running without VPP support
Sebastian Dröge [Tue, 3 Oct 2017 08:51:06 +0000 (10:51 +0200)]
vaapipostproc: Allow running without VPP support

We returned FALSE from ::start() if VPP support is not available, but it
is only really needed for complex filters and during transform we check
for that. For simple deinterlacing it is not needed.

6 years agovaapipostproc: use scoped variable for return value
Víctor Manuel Jáquez Leal [Wed, 27 Sep 2017 16:35:20 +0000 (18:35 +0200)]
vaapipostproc: use scoped variable for return value

Instead of reusing a parameter variable for the return value of
gst_vaapipostproc_transform_caps(), this patch uses the function
scoped pointer. Thus, the code is cleaner.

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

6 years agovaapipostproc: removed unused parameter
Víctor Manuel Jáquez Leal [Wed, 27 Sep 2017 16:32:03 +0000 (18:32 +0200)]
vaapipostproc: removed unused parameter

Removed caps parameter from gst_vaapipostproc_transform_caps_impl()
helper function since the it is not used.

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

6 years agovaapipostproc: use scoped variable for return value
Víctor Manuel Jáquez Leal [Wed, 27 Sep 2017 11:32:34 +0000 (13:32 +0200)]
vaapipostproc: use scoped variable for return value

Instead of reusing a parameter variable for the return value of
gst_vaapipostproc_fixate_caps(), this patch uses the function scoped
pointer. Thus, the code is cleaner.

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

6 years agoplugins: memory:DMABuf only handles planar formats
Víctor Manuel Jáquez Leal [Wed, 27 Sep 2017 09:27:40 +0000 (11:27 +0200)]
plugins: memory:DMABuf only handles planar formats

When glimagesink negotiates the caps feature memory:DMABuf the
exported dmabufs buffers with NV12 format are not well rendered, thus
setting only planar.

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

6 years agovaapiencode: flush pending frames before set format
Víctor Manuel Jáquez Leal [Mon, 25 Sep 2017 15:04:12 +0000 (17:04 +0200)]
vaapiencode: flush pending frames before set format

Flush pending frames, if any, in the internal encorder, before setting
the new negotiated format.

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

6 years agovaapidecode: drain pending frames before set format
Víctor Manuel Jáquez Leal [Mon, 25 Sep 2017 13:50:19 +0000 (15:50 +0200)]
vaapidecode: drain pending frames before set format

Drain pending frames, if any, in the internal decoder before setting
the new negotiated format.

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

6 years agotests: display: use GObject getter
Víctor Manuel Jáquez Leal [Fri, 22 Sep 2017 17:35:04 +0000 (19:35 +0200)]
tests: display: use GObject getter

Instead of using the gst_vaapi_display_get_property(), this patch
replaces it with g_object_get_property() to dump the available VA
display properties.

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

6 years agovaapisink: use GObject setter and getter
Víctor Manuel Jáquez Leal [Fri, 22 Sep 2017 17:25:20 +0000 (19:25 +0200)]
vaapisink: use GObject setter and getter

Instead of using gst_vaapi_display_set_property() or
gst_vaapi_display_get_property(), this patch set replace it usage
with g_object_set() or g_object_get().

Also the internal helper cb_set_value() is removed since it is not
used anymore.

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

6 years agolibs: display: initialize value if they are not yet
Víctor Manuel Jáquez Leal [Fri, 22 Sep 2017 16:59:49 +0000 (18:59 +0200)]
libs: display: initialize value if they are not yet

This is a difference between the GObject API and the GstVaapi one: the
GValue passed to get a property value, in GObject has to be
initialized with g_value_init(), but in GstVaapi is has not.

In order to overcome this mismatch, this patch call g_value_init()
internally only in the passed one is not already initialized.

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

6 years agolibs: display: optimize properties setters and getters
Víctor Manuel Jáquez Leal [Fri, 22 Sep 2017 15:04:17 +0000 (17:04 +0200)]
libs: display: optimize properties setters and getters

Shuffled some code to avoid to find the properties descriptor in the
array twice, adding the internal functions _set_property() and
_get_property().

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

6 years agolibs: display: install properties in class
Víctor Manuel Jáquez Leal [Fri, 22 Sep 2017 14:29:02 +0000 (16:29 +0200)]
libs: display: install properties in class

Install the properties in the class as a normal GObject. Implement
set_property() and get_property() vmethods.

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

6 years agolibs: display: remove gst_vaapi_display_properties_init()
Víctor Manuel Jáquez Leal [Fri, 22 Sep 2017 13:16:34 +0000 (15:16 +0200)]
libs: display: remove gst_vaapi_display_properties_init()

Remove gst_vaapi_display_properties_init() since it can be unrolled in
gst_vaapi_display_class_init()

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

6 years agolibs: display: remove libgstvaapi_init_once()
Víctor Manuel Jáquez Leal [Fri, 22 Sep 2017 13:12:05 +0000 (15:12 +0200)]
libs: display: remove libgstvaapi_init_once()

It is not required since it can be unrolled in
gst_vaapi_display_class_init()

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

6 years agotests: test-display: remove display cache tests
Víctor Manuel Jáquez Leal [Fri, 22 Sep 2017 15:50:15 +0000 (17:50 +0200)]
tests: test-display: remove display cache tests

Since commit ec3e10f6, display cache was removed. This patch removes
this leftovers in the display test.

6 years agolibs: decoder: h264/h265: decode codec data only if opened
Hyunjun Ko [Mon, 18 Sep 2017 05:29:55 +0000 (14:29 +0900)]
libs: decoder: h264/h265: decode codec data only if opened

Fixes regression introduced by commit 2eb2b26a.

There is a use case when the decoder set the src caps and immediatly
tries to process the media codec_data, this happens before decoder is
even opened, thus priv->parser is not instantiated yet.

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

6 years agolibs: encoder: change mbbrc from uint to enum
Víctor Manuel Jáquez Leal [Mon, 18 Sep 2017 17:11:45 +0000 (19:11 +0200)]
libs: encoder: change mbbrc from uint to enum

Instead of handling the macroblock bitrate control as a integer, this
patch changes it as a enum, which is more self documented in the
GStreamer elements.

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

6 years agoFix a typo in the prop string for compliance-mode
Jan Schmidt [Mon, 18 Sep 2017 03:55:49 +0000 (13:55 +1000)]
Fix a typo in the prop string for compliance-mode

6 years agolibs: encoder: don't unref properties
Víctor Manuel Jáquez Leal [Fri, 15 Sep 2017 16:31:49 +0000 (18:31 +0200)]
libs: encoder: don't unref properties

This patch fixes a regression introduced in commit 148f867c, since the
props variable is set to object's member variable
encoder->properties. And it is set in the instance initialization,
thus it will not be leaked.

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

6 years agovaapiencode/libs: encoder: fix leaks of properties
Hyunjun Ko [Fri, 15 Sep 2017 06:14:47 +0000 (15:14 +0900)]
vaapiencode/libs: encoder: fix leaks of properties

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

6 years agolibs: decoder: at update_caps() decode codec_data
Víctor Manuel Jáquez Leal [Thu, 24 Aug 2017 19:51:22 +0000 (21:51 +0200)]
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.

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

6 years agolibs: context: fix wrong counter of the array of attributes
Hyunjun Ko [Wed, 13 Sep 2017 06:44:32 +0000 (15:44 +0900)]
libs: context: fix wrong counter of the array of attributes

The counter value passed to vaCreateConfig is always +1.

This is a regression caused by commit e42ec3ad.

The present patch fixes wrong counting of the array of attributes.

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

6 years agolibs: encoder: h265: support I/P/B QP setting seperatedly
Hyunjun Ko [Wed, 13 Sep 2017 03:23:42 +0000 (12:23 +0900)]
libs: encoder: h265: support I/P/B QP setting seperatedly

Creates 2 properties, qp-ip and qp-ib for setting different QP for P/B
frames
and set slice_qp_delta for each frame according to the value provided.

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

6 years agolibs: encoder: h264: support I/P/B QP setting seperatedly
Hyunjun Ko [Wed, 13 Sep 2017 03:22:07 +0000 (12:22 +0900)]
libs: encoder: h264: support I/P/B QP setting seperatedly

Creates 2 properties, qp-ip and qp-ib for setting different QP for P/B
frames
and set slice_qp_delta for each frame according to the value provided.
In addition, remove the limitation of (<= 4) when setting
slice_qp_delta.

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

6 years agolibs: encoder: h264/h265: keep min_qp as is unless it's over init_qp
Hyunjun Ko [Wed, 13 Sep 2017 03:15:57 +0000 (12:15 +0900)]
libs: encoder: h264/h265: keep min_qp as is unless it's over init_qp

Creates new variable for QP for I frame and keep it at configuration and
use this for pic_init_qp and slice_qp_delta setting.

Since changing min qp doesn't make sense, keep min qp as is.

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

6 years agolibs: encoder: h265: Add mbbrc property
Hyunjun Ko [Wed, 13 Sep 2017 03:09:45 +0000 (12:09 +0900)]
libs: encoder: h265: Add mbbrc property

This property supports Macroblock level Bitrate Control as the
following (same as h264 encoder):
0: auto
1: on
2: off

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

6 years agolibs: encoder: h264: Add mbbrc property
Hyunjun Ko [Wed, 13 Sep 2017 03:02:53 +0000 (12:02 +0900)]
libs: encoder: h264: Add mbbrc property

This property supports Macroblock level Bitrate Control as the
following:
0: auto
1: on
2: off

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

6 years agolibs: encoder: h265: add multi reference support
Hyunjun Ko [Wed, 13 Sep 2017 02:39:09 +0000 (11:39 +0900)]
libs: encoder: h265: add multi reference support

This is doing the same as h264 encoder as the following:

Using num_ref_frames provided and the result of the Query
VAConfigAttribEncMaxRefFrames, it determines the size of reference list
and perform encoding with multi reference frames as the following:

1\ The num_ref_frames is being considered as the number of
reference picture list0
2\ Encoder adds 1 reference frame more to the reference picture list1
internally if b-frame encoding.
3\ If num_ref_frames is bigger than the number of refrence frames
supported in the driver, it will be lowered.

Also this patch includes:
- Set num_negative_pics and num_positive_pics according to the number of
refs.
- Set delta_poc according to the number of refs.
- Increase max_dec_pic_buffering according to the number of refs
- Change max_num_reorder_pics according to num of bframes

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

6 years agolibs: encoder: h265: add refs property
Hyunjun Ko [Wed, 13 Sep 2017 02:37:33 +0000 (11:37 +0900)]
libs: encoder: h265: add refs property

Users can provide the number of reference frame by this property,
which is exaclty same as h264.

The value of the property will be considered as the number of
reference picture list0 and will add 1 reference frame more to the
reference picture list1 internally if b-frame encoding.

If the value provided is bigger than the number of refrence frames
supported in the driver, it will be lowered.

The maximum value is aligned to the value of the driver supported now.

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

6 years agolibs: encoder: h264/5: determine num_ref_idx_active_override_flag according to refere...
Hyunjun Ko [Wed, 13 Sep 2017 02:17:26 +0000 (11:17 +0900)]
libs: encoder: h264/5: determine num_ref_idx_active_override_flag according to reference list

Follows the specification as below:

7.4.7.1 in Rec. ITU-T H.265 v4 (12/2016)
num_ref_idx_active_override_flag equal to 1 specifies that the syntax
element num_ref_idx_l0_active_minus1 is present for P and B slices and
that the syntax element num_ref_idx_l1_active_minus1 is present for B
slices.
num_ref_idx_active_override_flag equal to 0 specifies that the syntax
elements num_ref_idx_l0_active_minus1 and num_ref_idx_l1_active_minus1
are not present.

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

6 years agolibs: encoder: h265: keep idr_period equal to keyframe period
Hyunjun Ko [Wed, 13 Sep 2017 02:06:20 +0000 (11:06 +0900)]
libs: encoder: h265: keep idr_period equal to keyframe period

Remove FIXME code, which makes previous assignation spurious.
This also means to make idr_period equal to keyframe period,
which is same as h264 encoder.

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

6 years agoRequest minimum buffer even if need_pool is FALSE
Nicolas Dufresne [Wed, 6 Sep 2017 18:03:25 +0000 (14:03 -0400)]
Request minimum buffer even if need_pool is FALSE

When tee is used, it will not request a pool, but still it wants to
know how many buffers are required.

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

6 years agolibs: encoder: h264_fei: VA-API 1.0 compat
U. Artie Eoff [Tue, 5 Sep 2017 17:58:57 +0000 (10:58 -0700)]
libs: encoder: h264_fei: VA-API 1.0 compat

Use VA_ENC_PACKED_HEADER_H264_SEI compat macro for VA-API 1.0
compatibility.

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

Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
6 years agovaapisink: Fix rendering in drm display
Sreerenj Balachandran [Fri, 1 Sep 2017 20:48:01 +0000 (13:48 -0700)]
vaapisink: Fix rendering in drm display

Make sure vaapisink create a va surface backed buffer pool and all
required attributes get assigned correctly for drm display type.

This is needed to make the below pipeline working:
gst-launch-1.0 filesrc location= raw_video.mov ! videoparse format=uyvy
width=320 height=240 framerate=30/1 ! vaapisink display=drm

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

6 years agoFEI: Add test applications to showcase fei use case
Sreerenj Balachandran [Thu, 10 Aug 2017 01:46:09 +0000 (18:46 -0700)]
FEI: Add test applications to showcase fei use case

test-fei-enc-out: A simple fei encoding application to output mv, mbcode and distortion
eg:
 ./test-fei-enc-out -i sample_320x240.nv12 -w 320 -h 240 -o out.264 -v mv.out -d out.dist -m out.mbcode -e 1

test-fei-enc-in: A simple fei encoding application for testing input fei buffers
eg:
./test-fei-enc-in -c h264 -o out.264 -e 4 -q 1 sample_i420.y4m

Fixme: Running test-fei-enc-in in PAK mode with mv and mbcode input buffers
       from saved files is still not working

People contributed:
            Wang, Yi <yi.a.wang@intel.com>
            Leilei <leilei.shang@intel.com>
            Zhong, Xiaoxia <xiaoxia.zhong@intel.com>
            xiaominc <xiaomin.chen@intel.com>
            Li, Jing B <jing.b.li@intel.com>

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

6 years agoFEI: plugin: Add vaapih264feienc element
Sreerenj Balachandran [Thu, 10 Aug 2017 01:36:13 +0000 (18:36 -0700)]
FEI: plugin: Add vaapih264feienc element

A new FEI based encoder element for h264 is added: vaapih264feienc

FEI is a an extension to VA-API which is providing low level
advanced control over different stages of encoding.
Extending vaapih264enc with fei support is possible, but it will
make the code too much complicated and will be difficult
to debug. So adding the new encoder element, but keeping
the rank as 0 , vaapih264enc will stay as the primary
encoder for normal use cases.

The vaaih264feienc is mainly useful for customers who want to play
with MotionVectors and Macroblock Predictions. Also user can
do one stage of encoding(eg: only the Motion Vector Calculation)
in software and offload trasformation/entroy-coding etc to
Hardware (which is what PAK module is doing) using FEI element.

vaapih264feienc can work in  different modes using fei-mode properoty

eg: gst-launch-1.0 videotestsrc ! vaapih264feienc fei-mode=ENC+PAK ! filesink location=sample.264

Important Note: ENC only mode won't produce any encoded data which is expected.
But ENC alwys requires the output of PAK in order to do the inter-prediction
over reconstructed frames.
Similary PAK mode alway requires MV and MBCode as input, so unless there is an
upstream element providing those buffers, PAK only won't work as expected.

In a nutshell, ENC_PAK and the ENC+PAK modes are the only options we can verify
with vaapih264feienc. But ideally, EN+PAK mode verification is enough to make sure
that ENC and PAK are working as expected since ENC+PAK mode always invoke ENC and PAK
separately in vaapih264feienc.

People contributed:
        Wang, Yi <yi.a.wang@intel.com>
        Leilei <leilei.shang@intel.com>
        Zhong, Xiaoxia <xiaoxia.zhong@intel.com>
        xiaominc <xiaomin.chen@intel.com>
        Li, Jing B <jing.b.li@intel.com>

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

Signed-off-by: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
6 years agoFEI: plugin: Add virtual methods to base encode
Yi A Wang [Thu, 10 Aug 2017 01:32:13 +0000 (18:32 -0700)]
FEI: plugin: Add virtual methods to base encode

Two new virtual methods are added to gstvaapiencode.

load_control_data():  load the FEI input buffers set by the upstream elements
save_stats_to_meta(): save the FEI output buffers to Meta for downnstream elements

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

Signed-off-by: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
6 years agoFEI: plugin: Add fei specific video meta
Yi A Wang [Thu, 10 Aug 2017 01:26:57 +0000 (18:26 -0700)]
FEI: plugin: Add fei specific video meta

GstVaapiFeiVideoMeta holds the below fei codec objects:
  GstVaapiEncFeiMbCode
  GstVaapiEncFeiMv
  GstVaapiEncFeiMvPredictor
  GstVaapiEncFeiMbControl
  GstVaapiEncFeiQp
  GstVaapiEncFeiDistortion

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

Signed-off-by: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
6 years agoFEI: libs: Add FEI encoder
Sreerenj Balachandran [Thu, 10 Aug 2017 01:19:06 +0000 (18:19 -0700)]
FEI: libs: Add FEI encoder

Adding FEI encoder to core lib.

The code is splitted into three session:

1: gstvaapiencoder_h264_fei.{h,c}
This is the replica of gstvaapiencoder_h264.{c,h} but with FEI.
All the modes ENC, PAK and ENC_PAK are running based
the code in these files.

2: gstvaapifeienc_h264.{h,c}
Abstract implementation intended for ENC (only VME) operation.

3: gstvaapifeipak_h264.{h,c}
Abstrct implementation intended for PAK (only the PAK module)

Right now ENC_PAK, ENC and PAK are running based on code
in gstvaapiencoder_h264_fei.{h,c}. The abstract implementations
in gstvaapifeienc_h264.{h,c} and gstvaapifeipak_h264.{h,c} are
needed if user request for ENC+PAK mode operation.

ENC+PAK: Here we need to invoke two sequence of
vaBeginPicture/vaRenderPicutre/vaEndPicture for each frame,
first for the ENC only and the second for PAK only.
Each mode associated with separate context ,but same pool of surfaces are
shared between the modes.
This is more useful once we have custom BRC algorithms.

Other Contributors:
                   Wang, Yi <yi.a.wang@intel.com>
                   Leilei <leilei.shang@intel.com>
                   Zhong, Xiaoxia <xiaoxia.zhong@intel.com>
                   xiaominc <xiaomin.chen@intel.com>
                   Li, Jing B <jing.b.li@intel.com>

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

6 years agoFEI: libs: Add fei codec objects to GstVaapiEncPicture
Sreerenj Balachandran [Thu, 10 Aug 2017 00:54:27 +0000 (17:54 -0700)]
FEI: libs: Add fei codec objects to GstVaapiEncPicture

All the codec objects(vaapi buffers) supposed to be
submited in vaRenderPicutre are associated with a GstVaapiEncPicture
for each frame, follow the same design for FEI too.

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

6 years agoFEI: libs: Add fei codec objects in codedbufferproxy
Sreerenj Balachandran [Wed, 9 Aug 2017 23:05:13 +0000 (16:05 -0700)]
FEI: libs: Add fei codec objects in codedbufferproxy

MbCode, MV and Distortion buffers (fei codec objects)
can be treated as output of different fei modes based user request.
For eg: MbCode and MV are the output of ENC only. MbCode, MV and Dist
can be dumped as output in ENC_PAK mode for analysis purpose.
So treating them as a part of CodedBufferProxy too.
Here we avoided Qp, MbControl and MvPredictor codec objects since
there is no practical use case of treating them as "output buffers".

Other contributors:
               Zhong, Xiaoxia <xiaoxia.zhong@intel.com>
               xiaominc <xiaomin.chen@intel.com>
               Leilei <leilei.shang@intel.com>
               Li, Jing B <jing.b.li@intel.com>

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

6 years agoFEI: libs: Add fei codec objects to surface proxy
Sreerenj Balachandran [Wed, 9 Aug 2017 22:49:21 +0000 (15:49 -0700)]
FEI: libs: Add fei codec objects to surface proxy

Add fei codec objects to surface proxy since handling the
fei buffers(codec objects here) external to gstvaapisurfaceproxy
will make the code complicated. Especially considering the behavior
of encoder where the input frame order from upstream and output
frame order to the downstream are not sequential.

Other contributors:
              Zhong, Xiaoxia <xiaoxia.zhong@intel.com>
              xiaominc <xiaomin.chen@intel.com>
              Leilei <leilei.shang@intel.com>
              Li, Jing B <jing.b.li@intel.com>

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

6 years agoFEI: Add codec objects for fei usecase
Sreerenj Balachandran [Wed, 9 Aug 2017 22:35:10 +0000 (15:35 -0700)]
FEI: Add codec objects for fei usecase

There are 6 new va buffer types, each defined as a specific codec object.
Borrowed the code from gstvaapicodecobject , but made a clear separation
to avoid any possible mess-up. Because unlike the other gstvaaicodecobjects,
feicodecobjects can be shared between elements and also can be accessed
from different thread.

Unlike the other fei codecs object, VAEncMiscParameterTypeFEIFrameControl
object is not shared between elements.So we utilize the already
existing gst_vaapi_enc_misc_param_new(), but still keeping the code
in gstvaapfei_objects_priv.h in order to have a better
code readability.

Fixme:
-- Probably we need _locked_map() and _unlocked_map()
-- Context can be associated with PreEnc(not just Enoder)
once we have the proper support inplace, but for now we don't have
PreEnc support, so should be safe enough to use GstVaapiEncoder.

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

6 years agoFEI: libs: add H264 fei specific utility functions
Sreerenj Balachandran [Wed, 9 Aug 2017 21:22:12 +0000 (14:22 -0700)]
FEI: libs: add H264 fei specific utility functions

Added enum/flag type definitions for a number of FEI
input and output parameters.

Original author of the patch: Wang, Yi <yi.a.wang@intel.com>

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

Signed-off-by: Wang, Yi <yi.a.wang@intel.com>
Signed-off-by: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
6 years agoFEI: libs: Add virtual method for secondary context creation.
Sreerenj Balachandran [Wed, 9 Aug 2017 21:10:16 +0000 (14:10 -0700)]
FEI: libs: Add virtual method for secondary context creation.

Add a new vitrual method ensure_secondary_context to the
base encoder which is only required for the FEI entrypoint, that too
only when user configures the ENC+PAK mode. ENC+PAK mode is not something
supported directly by libva or driver, but this can be enabled
from the middleware.

Original Author of this idea: Leilei Shang <leilei.shang@intel.com>

Signed-off-by: Leilei Shang <leilei.shang@intel.com>
Signed-off-by: xiaominc <xiaomin.chen@intel.com>
Signed-off-by: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
https://bugzilla.gnome.org/show_bug.cgi?id=785712
https://bugzilla.gnome.org/show_bug.cgi?id=784667

6 years agoFEI: libs: make sure the default context creation works as expected.
Sreerenj Balachandran [Wed, 9 Aug 2017 21:05:03 +0000 (14:05 -0700)]
FEI: libs: make sure the default context creation works as expected.

Current code always guess the entrypoint during init phase in case
if there is no entrypoint already configured in GstVaapiContextInfo.
Make sure FEI Entrypoint is not messing up with this logic.

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

6 years agoFEI: libs: Add FEI functional mode configuration
Sreerenj Balachandran [Wed, 9 Aug 2017 20:45:40 +0000 (13:45 -0700)]
FEI: libs: Add FEI functional mode configuration

FEI Entrypoint can work in either one of the 3 different modes:
VA_FEI_FUNCTION_ENC, VA_FEI_FUNCTION_PAK or VA_FEI_FUNCTION_ENC_PAK.

Add infrastructure in gstvaapicontext and gstvaapiencoder for this
functioal mode configuration.

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

6 years agoFEI: libs: Add FEI Entrypoint mapping
Sreerenj Balachandran [Wed, 9 Aug 2017 20:02:24 +0000 (13:02 -0700)]
FEI: libs: Add FEI Entrypoint mapping

Define the new mapping GST_VAAPI_ENTRYPOINT_SLICE_ENCODE_FEI
for VAEntrypointFEI.

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

6 years agoFEI: Add support for FEI conditional build
Sreerenj Balachandran [Wed, 9 Aug 2017 19:58:29 +0000 (12:58 -0700)]
FEI: Add support for FEI conditional build

FEI(Flexible Encoding Infrastructure) is an extension
to VA API. Define USE_H264_FEI_ENCODER based on
fei header file and required structures availability.

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

6 years agovaapidecode: force add h264 SVC profiles in caps
Orestis Floros [Tue, 29 Aug 2017 00:34:50 +0000 (17:34 -0700)]
vaapidecode: force add h264 SVC profiles in caps

When vaapih264dec's base-only profile is set to TRUE, fake SVC profile
support in caps.

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

Signed-off-by: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
6 years agolibs: decoder: h264: decode SVC base layer only
Orestis Floros [Tue, 29 Aug 2017 00:32:57 +0000 (17:32 -0700)]
libs: decoder: h264: decode SVC base layer only

Drops non-base NALs when the base-only property is set to TRUE.
This modifies the behavior for MVC streams with base-only too: All the
non-base units are dropped before they are decoded instead of dropping
the non-base frames.

The relevant part from the H264 spec is:
> Decoders that conform to one or more of the profiles specified in
Annex A rather than the profiles specified in Annexes G or H shall
ignore (remove from the bitstream and discard) the contents of all NAL
units with nal_unit_type equal to 14, 15, or 20.

To eliminate side effects from the offending units:
- PPS's with a broken seq_parameter_set_id (referring to dropped subset
SPS's) are ignored.
- The NAL parsing is skipped and their flags are set to
GST_VAAPI_DECODER_UNIT_FLAG_SKIP.
- Prefix units are not stored in prev_pi. Otherwise, parse_slice() would
use them even if they are flagged to be skipped. Subset SPS's and slice
extension units are not stored there either.

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

Signed-off-by: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
6 years agolibs: decoder: h264: check nalu validity in parser info finalize
Orestis Floros [Tue, 29 Aug 2017 00:28:04 +0000 (17:28 -0700)]
libs: decoder: h264: check nalu validity in parser info finalize

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

Signed-off-by: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
6 years agolibs: encoder: remove unused cast macro
Víctor Manuel Jáquez Leal [Mon, 28 Aug 2017 17:20:42 +0000 (19:20 +0200)]
libs: encoder: remove unused cast macro

Remove internal macro to cast structure that are already declared
in the header.

6 years agoRevert "libs: encoders: remove unused cast macros"
Víctor Manuel Jáquez Leal [Mon, 28 Aug 2017 17:09:07 +0000 (19:09 +0200)]
Revert "libs: encoders: remove unused cast macros"

This reverts commit fd7d38f7d26b11e592638092b4073b5c1764f255.

6 years agolibs: encoders: remove unused cast macros
Víctor Manuel Jáquez Leal [Mon, 28 Aug 2017 16:32:32 +0000 (18:32 +0200)]
libs: encoders: remove unused cast macros

They are only used inside the code, where another macro is defined.
Thus these exported macros have no use.

6 years agolibs: decoder: h264: improve code-style
Víctor Manuel Jáquez Leal [Thu, 24 Aug 2017 18:26:11 +0000 (20:26 +0200)]
libs: decoder: h264: improve code-style

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

6 years agolibs: encoder: h264: handle deprecated enum
Víctor Manuel Jáquez Leal [Fri, 25 Aug 2017 14:22:59 +0000 (16:22 +0200)]
libs: encoder: h264: handle deprecated enum

In VA-API 1.0 the enum VAEncPackedHeaderH264_SEI is deprecated, and
instead VAEncPackedHeaderRawData should be used.

This patch creates a compatibility symbol,
VA_ENC_PACKED_HEADER_H264_SEI, to expose the used enum according the
VA-API version.

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

6 years agolibs: guard deprecated symbols
Víctor Manuel Jáquez Leal [Fri, 25 Aug 2017 14:07:34 +0000 (16:07 +0200)]
libs: guard deprecated symbols

In VA-API 1.0 the H.264 baseline profile is deprecated. This patch
guards the H.264 baseline usage. Consider this commit as a
continuation of commit e0e0a474

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

6 years agoRevert "build: check for libva-2.0"
Víctor Manuel Jáquez Leal [Thu, 17 Aug 2017 10:54:47 +0000 (12:54 +0200)]
Revert "build: check for libva-2.0"

This reverts commit 8f2eb70803099d4b533ecc10fc259041d8714210.

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

6 years agolibs: macro to get a renamed value in VA-API 1.0
Víctor Manuel Jáquez Leal [Thu, 17 Aug 2017 10:44:40 +0000 (12:44 +0200)]
libs: macro to get a renamed value in VA-API 1.0

In VA-API 1.0 the union bits in VAEncMiscParameterBufferROI has
renamed one member from roi_value_is_qp_delat to
roi_value_is_qp_delta, which is the correct name.

In order to keep back compatibility a macro has added to access this
union member.

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

6 years agoplugins: include main gstgl header
Víctor Manuel Jáquez Leal [Tue, 22 Aug 2017 09:37:28 +0000 (11:37 +0200)]
plugins: include main gstgl header

Instead including particular gstgl header files in a header file
that doesn't export a gstgl symbol, the main gstgl header file is
included in gstvaapipluginutil.c where the symbols are used.

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

6 years agolibs: encoder: h264: fix enum namespace
Víctor Manuel Jáquez Leal [Fri, 18 Aug 2017 16:00:24 +0000 (18:00 +0200)]
libs: encoder: h264: fix enum namespace

6 years agoAutomatic update of common submodule
Tim-Philipp Müller [Thu, 17 Aug 2017 11:26:12 +0000 (12:26 +0100)]
Automatic update of common submodule

From 48a5d85 to 3f4aa96

6 years agolibs: encoder: h264: remove spurious assignation
Víctor Manuel Jáquez Leal [Thu, 17 Aug 2017 09:03:35 +0000 (11:03 +0200)]
libs: encoder: h264: remove spurious assignation

Coverity scan bug:

An assigned value that is never used may represent unnecessary
computation, an incorrect algorithm, or possibly the need for cleanup
or refactoring.

ip_period is assigned first to be rewritter inmediatly after. The
first assignation is spurious.

6 years agovaapidecode: fix mismatch of the return type
Hyunjun Ko [Tue, 15 Aug 2017 08:36:51 +0000 (17:36 +0900)]
vaapidecode: fix mismatch of the return type

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

6 years agolibs: remove unused header
Víctor Manuel Jáquez Leal [Thu, 10 Aug 2017 11:34:21 +0000 (13:34 +0200)]
libs: remove unused header

Since libgstvaapi is not distributed, there is no need to check for
private header inclusion. Thus removing it.

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

6 years agolibs: utils: move gstvaapisurface.h to private headers
Víctor Manuel Jáquez Leal [Thu, 10 Aug 2017 11:27:11 +0000 (13:27 +0200)]
libs: utils: move gstvaapisurface.h to private headers

Since the utils don't expose API defined in gstvaapisource.h, it is
moved to their private headers where they are used.

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

6 years agolibs: utils: remove va.h include in header
Víctor Manuel Jáquez Leal [Thu, 10 Aug 2017 11:26:12 +0000 (13:26 +0200)]
libs: utils: remove va.h include in header

And include gstvaapicompat.h in the C files, since the VA-API is not
exposed in the headers.

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

6 years agolibs: encoder: remove va.h include
Víctor Manuel Jáquez Leal [Thu, 10 Aug 2017 11:24:06 +0000 (13:24 +0200)]
libs: encoder: remove va.h include

Since it is already managed by gstvaapicompat.h

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

6 years agobuild: consolidate the VA sub API includes
Víctor Manuel Jáquez Leal [Thu, 10 Aug 2017 11:11:04 +0000 (13:11 +0200)]
build: consolidate the VA sub API includes

Include all VA sub APIs headers in a single point (gstvaapicompat.h),
since they are all already included in va.h after VA-API 0.38.

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

6 years agobuild: check for va_vpp.h
Víctor Manuel Jáquez Leal [Thu, 10 Aug 2017 11:09:27 +0000 (13:09 +0200)]
build: check for va_vpp.h

Thus, in config.h the macro HAVE_VA_VA_VPP_H is defined. This will
allow us to handle the inclusion of the header better.

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

6 years agomeson: don't export symbols by default
Tim-Philipp Müller [Fri, 11 Aug 2017 19:22:41 +0000 (20:22 +0100)]
meson: don't export symbols by default

Only plugin entry points should be exported.

6 years agolibs: decoder: h265: remove spurious code
Víctor Manuel Jáquez Leal [Wed, 9 Aug 2017 17:06:59 +0000 (19:06 +0200)]
libs: decoder: h265: remove spurious code

Coverity scan:

Logically dead code: The indicated dead code may have performed some
action; that action will never occur.

By using pointer arithmetic is impossible to get NULL.

6 years agolibs: context: use attribs index instead pointers
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 16:52:37 +0000 (18:52 +0200)]
libs: context: use attribs index instead pointers

Coverity scan bug:

Out-of-bounds write. This could cause an immediate crash or incorrect
computations.

Coverity basically found that it is possible to assign more than 4
attribs in the array.

In my opinion this was produced because code pattern used pointer
arithmetic, which is not readable nor maintainable.

This patch refactors config_create() to use an array index rather than
pointer arithmetic. Also a run-time check for index size was added.

6 years agovaapiencode: h264: remove spurious code
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 15:38:51 +0000 (17:38 +0200)]
vaapiencode: h264: remove spurious code

Coverity scan bug:

An unsigned value can never be negative, so this test will always
evaluate the same way.

As len is guint32, there is no need to check it if it is equal or
bigger than zero.

6 years agovaapidecode: initialize variable
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 15:34:12 +0000 (17:34 +0200)]
vaapidecode: initialize variable

Coverity scan bug:

The variable will contain an arbitrary value left from earlier
computations.

Variable base_only is fetched from base-only property, and it may be
not assigned. It needs to be initialized.

6 years agolibs: windows: wayland: fail if cannot remove last frame
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 15:29:54 +0000 (17:29 +0200)]
libs: windows: wayland: fail if cannot remove last frame

Converity scan bug:

If the function returns an error value, the error value may be
mistaken for a normal value.

If g_atomic_pointer_compare_and_exchange() fails because the frame is
not the last one, the function fails. Thus, logging an info message.

6 years agolibs: utils: glx: check return value
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 15:21:52 +0000 (17:21 +0200)]
libs: utils: glx: check return value

Coverity scan bug:

If the function returns an error value, the error value may be
mistaken for a normal value.

Function sscanf returns the number of assignations done. Validate this
return value with the number of expected variables to match.

6 years agolibs: vaapi: object: remove unrequired NULL check
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 15:12:06 +0000 (17:12 +0200)]
libs: vaapi: object: remove unrequired NULL check

Coverity scan bug:

Dereference after null check: Either the check against null is
unnecessary, or there may be a null pointer dereference.

Variable klass has been validated as non-NULL several time before in
gst_vaapi_object_new() function, so there is no need to check it
again.

6 years agolibs: encoder: h265: remove spurious assignation
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 15:06:17 +0000 (17:06 +0200)]
libs: encoder: h265: remove spurious assignation

Coverity scan bug:

An assigned value that is never used may represent unnecessary
computation, an incorrect algorithm, or possibly the need for cleanup
or refactoring.

ip_period is assigned first to be rewritter inmediatly after. The
first assignation is spurious.

6 years agolibs: encoder: h264: fix copy & paste error
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 14:50:39 +0000 (16:50 +0200)]
libs: encoder: h264: fix copy & paste error

Coverity scan bug:

The copied code will not have its intended effect.

This is a bug from commit cdaf15b2, where the intention is to
initialize RefPicList1 while setting RefPicList0.

6 years agolibs: encoder: h265: fix possible integer overflow
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 14:33:44 +0000 (16:33 +0200)]
libs: encoder: h265: fix possible integer overflow

Coverity scan bug:

Unintentional integer overflow. The expression's value may not be what
the programmer intended, because the expression is evaluated using a
narrow (i.e. few bits) integer type.

Cast operator to guint64 before computation to avoid narrowing.

merge with 3c5a6add

6 years agolibs: decoder: mpeg4: fail if return value is not OK
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 14:12:13 +0000 (16:12 +0200)]
libs: decoder: mpeg4: fail if return value is not OK

Coverity scan bug:

An assigned value that is never used may represent unnecessary
computation, an incorrect algorithm, or possibly the need for cleanup
or refactoring.

In the return value of decode_slice() or
gst_mpeg4_parse_video_packet_header() are not success, thus fail
decode_packet() function.

6 years agolibs: decoder: h265: check for null
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 13:49:27 +0000 (15:49 +0200)]
libs: decoder: h265: check for null

Coverity scan bug:

Dereference after null check: Either the check against null is
unnecessary, or there may be a null pointer dereference.

While looking for hte lowest poc, according to rest of the code, the
picture in the dbp (decoded picture buffer) might be NULL, thus we
could check for a NULL picture before assigned as found.

Also, split a comma operator because it is considered as a bad
practice because it possible side effects.

6 years agolibs: decoder: h265: untaint loop control variable
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 13:38:16 +0000 (15:38 +0200)]
libs: decoder: h265: untaint loop control variable

Coverity scan bug:

Scalars (for example, integers) are not properly
bounds-checked (sanitized) before being used as array or pointer
indexes, loop boundaries, or function arguments are considered as
tainted.

In this case, num_nals were not checked before used as loop control.

6 years agolibs: decoder: h264: remove unrequired NULL check
Víctor Manuel Jáquez Leal [Tue, 8 Aug 2017 11:46:56 +0000 (13:46 +0200)]
libs: decoder: h264: remove unrequired NULL check

Coverity scan bug:

Dereference after null check: Either the check against null is
unnecessary, or there may be a null pointer dereference.

In the original commit for fill_picture_gaps() (commit 5abd2b90) the
prev_picture could be NULL, that's why the code did a null check. But,
since commit 52adebe7, the previous reference frames are tracked, thus
there is no need to check null anymore.

6 years agovaapidecode: fix gst_caps_new_simple call
orestisf [Thu, 3 Aug 2017 20:17:44 +0000 (23:17 +0300)]
vaapidecode: fix gst_caps_new_simple call

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

6 years agovaapidecode: force add h264 MVC profiles in caps
orestisf [Tue, 25 Jul 2017 19:25:10 +0000 (22:25 +0300)]
vaapidecode: force add h264 MVC profiles in caps

When vaapih264dec's base-only profile is set to TRUE, fake MVC profile
support in caps.

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

6 years agolibs: decoder: h264: decode MVC base view only
orestisf [Tue, 25 Jul 2017 19:54:30 +0000 (22:54 +0300)]
libs: decoder: h264: decode MVC base view only

If processed SPS has mvc profile and the configuration is set to
base-only, the frame is drop.

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

6 years agovaapidecode: set h264 base-only to decoder
orestisf [Tue, 25 Jul 2017 19:06:56 +0000 (22:06 +0300)]
vaapidecode: set h264 base-only to decoder

Set the base-only value when property is set and the internal
decoder is already instantiated or when the internal decoder
is created.

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

6 years agolibs: decoder: h264: add setter for base-only mode
orestisf [Tue, 25 Jul 2017 19:03:34 +0000 (22:03 +0300)]
libs: decoder: h264: add setter for base-only mode

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

6 years agovaapidecode_props: h264: add base-only property
orestisf [Tue, 25 Jul 2017 19:01:37 +0000 (22:01 +0300)]
vaapidecode_props: h264: add base-only property

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

6 years agolibs: encoder: h264: missing property enum documentation
Víctor Manuel Jáquez Leal [Tue, 1 Aug 2017 09:11:55 +0000 (11:11 +0200)]
libs: encoder: h264: missing property enum documentation

6 years agolibs: encoder: h264: add multi reference support
Hyunjun Ko [Wed, 2 Aug 2017 05:54:53 +0000 (14:54 +0900)]
libs: encoder: h264: add multi reference support

Using num_ref_frames provided and the result of the Query
VAConfigAttribEncMaxRefFrames, it determines the size of reference list
and perform encoding with multi reference frames as the following:

1\ The num_ref_frames is being considered as the number of
reference picture list0
2\ Encoder adds 1 reference frame more to the reference picture list1
internally if b-frame encoding.
3\ If num_ref_frames is bigger than the number of refrence frames
supported in the driver, it will be lowered.

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

6 years agolibs: encoder: h264: add refs property
Hyunjun Ko [Wed, 2 Aug 2017 05:53:34 +0000 (14:53 +0900)]
libs: encoder: h264: add refs property

Users can provide the number of reference frame by this property.

The value of the property will be considered as the number of
reference picture list0 and will add 1 reference frame more to the
reference picture list1 internally if b-frame encoding.

If the value provided is bigger than the number of refrence frames
supported in the driver, it will be lowered.

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

6 years agolibs: encoder: implements gst_vaapi_encoder_ensure_max_num_ref_frames
Hyunjun Ko [Fri, 28 Jul 2017 06:27:20 +0000 (15:27 +0900)]
libs: encoder: implements gst_vaapi_encoder_ensure_max_num_ref_frames

This function will query VAConfigAttribEncMaxRefFrames to get the
maximum number of reference frames supported in the driver.
This will be used for h264/h265 encoding.

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

6 years agovaapiencode: h265: compare an unsigned int if not zero
Víctor Manuel Jáquez Leal [Tue, 1 Aug 2017 16:38:40 +0000 (18:38 +0200)]
vaapiencode: h265: compare an unsigned int if not zero

An unsigned value can never be negative, so this test (greater than
zero) will always evaluate the same way. Thus change it to just if
it's not zero.