GNOME Bugzilla – Bug 796705
vaapijpegenc: Encoding JPEG will result in green/black display encoded output
Last modified: 2018-07-04 10:50:31 UTC
[System Environment] Platform : APL Kernel : 4.17-rc3 libva : master branch d6fd111e2062bb4732db8a05ed55fc01771087b4 gst-vaapi : 1.14.0 [Defect description] Encoding JPEG looks successful either using raw file or videotestsrc. However, encoded content was showing green (using glimagesink) or black (using vaapisink) screen when trying to playback. [Reproduce Steps] Encoded command: raw input file $ gst-launch-1.0 -v filesrc location=/home/root/videos/raw/NV12_Prog_Planar_720x480.yuv ! videoparse width=720 height=480 format=nv12 ! vaapijpegenc quality=100 ! avimux ! filesink location=test.avi videotestsrc $ gst-launch-1.0 -v videotestsrc num-buffers=100 ! video/x-raw,format=NV12 ! vaapijpegenc ! avimux ! filesink location=test.avi Playback command: glimagesink $ GST_GL_PLATFORM=egl gst-launch-1.0 filesrc location= /media/test.avi ! avidemux ! vaapijpegdec ! vaapipostproc ! glimagesink vaapisink $ gst-launch-1.0 filesrc location= /media/test.avi ! avidemux ! vaapijpegdec ! vaapipostproc ! vaapisink
Created attachment 372862 [details] [review] set component identifier and quantization table destination selector in frame header
Victor, could you review my patches?
Review of attachment 372862 [details] [review]: Overall it looks good. Just a couple nitpicks: 1. The commit log format: the subject, if possible, should be 50 chars long, with prefix "libs: encoder: jpeg: ..." 2. Wrap the body to 72 chars https://chris.beams.io/posts/git-commit/#seven-rules In my opinion is important to remark the importance of the patch regardless the va backend, setting the component id and Tqi ::: gst-libs/gst/vaapi/gstvaapiencoder_jpeg.c @@ +229,3 @@ + pic_param->component_id[i] = i + 1; + if (i == 0) + pic_param->quantiser_table_selector[i] = 0; There is no need to set zero, since the structure is already set to zero.
Created attachment 372909 [details] [review] set component identifier and quantization table destination selector in frame header
(In reply to Víctor Manuel Jáquez Leal from comment #3) > Review of attachment 372862 [details] [review] [review]: > > Overall it looks good. > > Just a couple nitpicks: > > 1. The commit log format: the subject, if possible, should be 50 chars long, > with prefix "libs: encoder: jpeg: ..." > 2. Wrap the body to 72 chars > > https://chris.beams.io/posts/git-commit/#seven-rules > > In my opinion is important to remark the importance of the patch regardless > the va backend, setting the component id and Tqi > > ::: gst-libs/gst/vaapi/gstvaapiencoder_jpeg.c > @@ +229,3 @@ > + pic_param->component_id[i] = i + 1; > + if (i == 0) > + pic_param->quantiser_table_selector[i] = 0; > > There is no need to set zero, since the structure is already set to zero. Thanks for your review, I'll write a good message and submit a new patch.
I change my according your comments. Victor, could you review my patch?
Ideally, we should use the prefilled values in VAEnc* structures (if it is available) to fill up any PackedHeader attributes so that we don't accidentally mess up with different values in both places. The same pattern we have used in all encoder packed header generation. Here, I would recommend you to use VAEncPictureParameterBufferJPEG's component_id and quantiser_table_selector (which are prefilled by your patch) for packed frame header generation later on.
(In reply to sreerenj from comment #7) > Ideally, we should use the prefilled values in VAEnc* structures (if it is > available) to fill up any PackedHeader attributes so that we don't > accidentally mess up with different values in both places. The same pattern > we have used in all encoder packed header generation. > > Here, I would recommend you to use VAEncPictureParameterBufferJPEG's > component_id and quantiser_table_selector (which are prefilled by your > patch) for packed frame header generation later on. Thanks, I'll follow your recommendation and submit a new patch
Created attachment 372946 [details] [review] set component identifier and quantization table destination selector in frame header
Review of attachment 372946 [details] [review]: lgtm