GNOME Bugzilla – Bug 768458
vaapiencode: use gst_util_uint64_scale() and check fps numerator
Last modified: 2016-08-10 10:47:13 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?
Created attachment 330917 [details] [review] RFC: vaapiencode: h264: check fps numerator
(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...
(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().
@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.
(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 ?
(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.
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.
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.
(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
(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 :)
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.
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