After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 768458 - vaapiencode: use gst_util_uint64_scale() and check fps numerator
vaapiencode: use gst_util_uint64_scale() and check fps numerator
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
unspecified
Other All
: Normal normal
: 1.9.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-07-05 18:10 UTC by Víctor Manuel Jáquez Leal
Modified: 2016-08-10 10:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
RFC: vaapiencode: h264: check fps numerator (980 bytes, patch)
2016-07-05 18:10 UTC, Víctor Manuel Jáquez Leal
none Details | Review
vaapiencode: h264,h265: validate fps numerator (2.47 KB, patch)
2016-07-22 09:34 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
encoder: mpeg2,vp8: use gst_util_uint64_scale() (2.51 KB, patch)
2016-07-22 09:34 UTC, Víctor Manuel Jáquez Leal
none Details | Review
encoder: h264,h265,mpeg2,vp8: use gst_util_uint64_scale() for bitrate (4.46 KB, patch)
2016-07-26 14:31 UTC, Víctor Manuel Jáquez Leal
committed Details | Review

Description Víctor Manuel Jáquez Leal 2016-07-05 18:10:08 UTC
Doing some test with vaapih264enc I found that if the negotiated caps has a 
framerate of 0/1, the pipeline crash with an arithmetic fault.

This is a simplistic solution, but I don't know if it is the right solution,
perhaps we should change that when negotiating the caps.

@Sree, what do you think?
Comment 1 Víctor Manuel Jáquez Leal 2016-07-05 18:10:12 UTC
Created attachment 330917 [details] [review]
RFC: vaapiencode: h264: check fps numerator
Comment 2 sreerenj 2016-07-06 10:38:25 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #1)
> Created attachment 330917 [details] [review] [review]
> RFC: vaapiencode: h264: check fps numerator

fps checking is already done in gstvaapiencoder, isn't it ?
so how the code reach here? , if it is reaching here, then there should be other failure cases too, for eg do "vaapiencode rate-control=cbr bitrate=4096" , it should also break...
Comment 3 Víctor Manuel Jáquez Leal 2016-07-21 14:48:47 UTC
(In reply to sreerenj from comment #2)
> (In reply to Víctor Manuel Jáquez Leal from comment #1)
> > Created attachment 330917 [details] [review] [review] [review]
> > RFC: vaapiencode: h264: check fps numerator
> 
> fps checking is already done in gstvaapiencoder, isn't it ?
> so how the code reach here? , if it is reaching here, then there should be
> other failure cases too, for eg do "vaapiencode rate-control=cbr
> bitrate=4096" , it should also break...

Sorry for the delay.

Confirmed: FPS is *not* verified in gstvaapiencoder.

You can reproduce the bug with attachment 325373 [details]

gst-launch-1.0 filesrc location= ~/patterns/bug764607.mjpg ! decodebin ! vaapih264enc ! fakesink -v
Setting pipeline to PAUSED ...
libva info: VA-API version 0.39.2
libva info: va_getDriverName() returns 0
libva info: Trying to open /opt/gnome/jh/lib/dri/i965_drv_video.so
libva info: Found init function __vaDriverInit_0_39
libva info: va_openDriver() returns 0
Pipeline is PREROLLING ...
Got context from element 'vaapiencodeh264-0': gst.vaapi.Display=context, gst.vaapi.Display=(GstVaapiDisplay)NULL;
/GstPipeline:pipeline0/GstDecodeBin:decodebin0/GstTypeFindElement:typefind.GstPad:src: caps = image/jpeg, width=(int)1280, height=(int)720, sof-marker=(int)0
/GstPipeline:pipeline0/GstDecodeBin:decodebin0/GstVaapiDecode_jpeg:vaapidecode_jpeg0.GstPad:sink: caps = image/jpeg, width=(int)1280, height=(int)720, sof-marker=(int)0
Redistribute latency...
/GstPipeline:pipeline0/GstDecodeBin:decodebin0/GstVaapiDecode_jpeg:vaapidecode_jpeg0.GstPad:src: caps = video/x-raw(memory:VASurface), format=(string)NV12, width=(int)1280, height=(int)720, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1, framerate=(fraction)0/1
Floating point exception
 
But the patch is not correct. We should use gst_util_uint64_scale().
Comment 4 Víctor Manuel Jáquez Leal 2016-07-21 15:19:18 UTC
@sree,

Another question, is correct

     encoder->cts_offset = GST_SECOND * GST_VAAPI_ENCODER_FPS_D (encoder) /
         GST_VAAPI_ENCODER_FPS_N (encoder);

???

cts_offset is used to calculate frame->pts (composition timestamp for presentation timestamp)

But D/N doesn't look right to me. It should be N/D, but I would like your confirmation.

This code is also used in h265 encoding.
Comment 5 sreerenj 2016-07-22 08:39:52 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #4)
> @sree,
> 
> Another question, is correct
> 
>      encoder->cts_offset = GST_SECOND * GST_VAAPI_ENCODER_FPS_D (encoder) /
>          GST_VAAPI_ENCODER_FPS_N (encoder);
> 
> ???
> 
> cts_offset is used to calculate frame->pts (composition timestamp for
> presentation timestamp)
> 
> But D/N doesn't look right to me. It should be N/D, but I would like your
> confirmation.
> 
> This code is also used in h265 encoding.

This code block is only adding an extra frame duration/delay if there is b frames included in the stream. one frame duration = D/N , missing something ?
Comment 6 Víctor Manuel Jáquez Leal 2016-07-22 08:53:35 UTC
(In reply to sreerenj from comment #5)
> (In reply to Víctor Manuel Jáquez Leal from comment #4)
> > @sree,
> > 
> > Another question, is correct
> > 
> >      encoder->cts_offset = GST_SECOND * GST_VAAPI_ENCODER_FPS_D (encoder) /
> >          GST_VAAPI_ENCODER_FPS_N (encoder);
> > 
> > ???
> > 
> > cts_offset is used to calculate frame->pts (composition timestamp for
> > presentation timestamp)
> > 
> > But D/N doesn't look right to me. It should be N/D, but I would like your
> > confirmation.
> > 
> > This code is also used in h265 encoding.
> 
> This code block is only adding an extra frame duration/delay if there is b
> frames included in the stream. one frame duration = D/N , missing something ?

Thanks for the explanation! :)

Ok. Then let me upload the patches for your review.
Comment 7 Víctor Manuel Jáquez Leal 2016-07-22 09:34:18 UTC
Created attachment 331968 [details] [review]
vaapiencode: h264,h265: validate fps numerator

Validate that fps numerator is non-zero so it can be used to calculate
the duration of the B frame.

Also it gst_util_uint64_scale() is used instead of normal arithmetic in
order to aviod overflows, underflows and loss of precision.
Comment 8 Víctor Manuel Jáquez Leal 2016-07-22 09:34:22 UTC
Created attachment 331969 [details] [review]
encoder: mpeg2,vp8: use gst_util_uint64_scale()

Use gst_util_uint64_scale() to calculate bitrate instead of normal arithmetic
to avoid overflows, underflows and loss of precision.
Comment 9 sreerenj 2016-07-26 12:50:19 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #7)
> Created attachment 331968 [details] [review] [review]
> vaapiencode: h264,h265: validate fps numerator
> 
> Validate that fps numerator is non-zero so it can be used to calculate
> the duration of the B frame.
> 
> Also it gst_util_uint64_scale() is used instead of normal arithmetic in
> order to aviod overflows, underflows and loss of precision.

I am fine with the patch, But wondering whether we need this offset addition.

x264 enc with 1 bframe: pts of output frames (why 1000.xxx.xx??? )
pts: 1000:00:00.000000000
pts: 1000:00:00.066666666
pts: 1000:00:00.033333333

vaapih264enc with 1 bframe: pts of output frames:
pts: 0:00:00.033333333
pts: 0:00:00.099999999
pts: 0:00:00.066666666
Comment 10 sreerenj 2016-07-26 12:53:49 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #8)
> Created attachment 331969 [details] [review] [review]
> encoder: mpeg2,vp8: use gst_util_uint64_scale()
> 
> Use gst_util_uint64_scale() to calculate bitrate instead of normal arithmetic
> to avoid overflows, underflows and loss of precision.

There are same bitrate calculation code in h264 and h265 too :)
Comment 11 Víctor Manuel Jáquez Leal 2016-07-26 14:31:59 UTC
Created attachment 332143 [details] [review]
encoder: h264,h265,mpeg2,vp8: use gst_util_uint64_scale() for bitrate

Use gst_util_uint64_scale() to calculate bitrate instead of normal arithmetic
to avoid overflows, underflows and loss of precision.
Comment 12 Víctor Manuel Jáquez Leal 2016-08-10 10:44:16 UTC
Attachment 331968 [details] pushed as 4259d1a - vaapiencode: h264,h265: validate fps numerator
Attachment 332143 [details] pushed as 68b9ab7 - encoder: h264,h265,mpeg2,vp8: use gst_util_uint64_scale() for bitrate