GNOME Bugzilla – Bug 754024
codecparsers: Various HEVC codecparser fixes
Last modified: 2015-08-28 18:28:12 UTC
Couple of hevc parsing fixes: -- codecparsers: h265: Fix the range of delta_chroma_log2_weight_denom -- codecparsers: h265: Fix tile row and column parsing -- codecparsers: h265: Add APIs for up-right-diagonal/raster scan conversion
Created attachment 309914 [details] [review] codecparsers: h265: Fix the range of delta_chroma_log2_weight_denom
Created attachment 309915 [details] [review] codecparsers: h265: Fix tile row and column parsing
Created attachment 309917 [details] [review] codecparsers: h265: Add APIs for up-right-diagonal/raster scan conversion
Patch1: Follow the spec as exactly for checking the range patch2: This is a necessary patch, with out this the tile_row and tile_column values calculated are wrong. I have done this fix in gstremaer-vaapi months ago , but forgot to move this to codecparsers. patch3: This is required for projects gstreamer-vaapi and libyami. But nothing touching the existing parsing code. Only adding few more APIs to do up-right-diagonal to raster scan convertion and vice versa.
Review of attachment 309915 [details] [review]: Seems ok to me (without having the spec) but ::: gst-libs/gst/codecparsers/gsth265parser.c @@ +1805,3 @@ + ceil ((gdouble) sps->pic_height_in_luma_samples / (gdouble) CtbSizeY); + pps->PicWidthInCtbsY = + ceil ((gdouble) sps->pic_width_in_luma_samples / (gdouble) CtbSizeY); Can we have integer arithmetic here? :)
commit 9f5d96271964d1ccab9272b4efa91f98ba099f29 Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon Aug 24 21:28:46 2015 +0300 win32: Add new h265 parser symbols commit 497f3726687bac932eaf7313b9f0d31800ff473b Author: lyb <andy_liu_china@163.com> Date: Mon Aug 24 08:06:15 2015 +0300 codecparsers: h265: Add APIs for up-right-diagonal/raster scan conversion As per 7-42 and 7-43 the ScalingFactor's scanIdx is 0, which is "up-right-diagonal" scan. Add APIs for converting up-right-diagonal to raster and vise versa. https://bugzilla.gnome.org/show_bug.cgi?id=754024 commit 26e350f0bf865594315f7af194d2a5a980e00f41 Author: XuGuangxin <Guangxin.Xu@intel.com> Date: Mon Aug 24 04:18:52 2015 +0300 codecparsers: h265: Fix the range of delta_chroma_log2_weight_denom Being more strict on specification, According to 7.4.7.3, delta_chroma_log2_weight_denom should be in the range of [(0 - luma_log2_weight_denom), (7 - luma_log2_weight_denom)] https://bugzilla.gnome.org/show_bug.cgi?id=754024
(In reply to Sebastian Dröge (slomo) from comment #5) > Review of attachment 309915 [details] [review] [review]: > > Seems ok to me (without having the spec) but > > ::: gst-libs/gst/codecparsers/gsth265parser.c > @@ +1805,3 @@ > + ceil ((gdouble) sps->pic_height_in_luma_samples / (gdouble) CtbSizeY); > + pps->PicWidthInCtbsY = > + ceil ((gdouble) sps->pic_width_in_luma_samples / (gdouble) CtbSizeY); > > Can we have integer arithmetic here? :) As per spec "% Used to denote division in mathematical equations where no truncation or rounding is intended." and PicWidthInCtbsY= Ceil (sps->pic_width_in_luma_samples % CtbSizeY) Do you want to get a custom ceil function to get this job done with integer arithmetic??
Aha may be I can use the gst_util_uint64_scale_ceil (),,,!
Created attachment 310006 [details] [review] codecparsers: h265: Fix tile row and column parsing
I have noticed that you removed the Signed-off message!, I wonder why? I don't have commit access, at least I should get the signed-off signature since I am spending significant time on making these patches upstream compatible (on-line and offline),proper commit message etc etc... :)
(In reply to sreerenj from comment #10) > I have noticed that you removed the Signed-off message!, I wonder why? > I don't have commit access, at least I should get the signed-off signature > since I am spending significant time on making these patches upstream > compatible (on-line and offline),proper commit message etc etc... :) We generally don't do this signed-off stuff, that's why I removed it. I didn't notice that the patches were actually not from you, but you forwarded them for someone else. I guess for such a case it makes sense to keep it, sorry for removing :)
Or just do > (sps->pic_height_in_luma_samples + CtbSizeY - 1) / CtbSizeY I'll merge that patch for now as is, as it just moves the code around anyway. But please provide a patch for later that gets rid of the floating point arithmetic :)
Comment on attachment 310006 [details] [review] codecparsers: h265: Fix tile row and column parsing For later, no need to go via 64 bit (actually 96 bit) integers here :)
commit 54c2620bdb256022cdd11dd6639ca91cb982ffe2 Author: XuGuangxin <Guangxin.Xu@intel.com> Date: Mon Aug 24 07:46:27 2015 +0300 codecparsers: h265: Fix tile row and column parsing Section 6.5.1: Coding tree block raster and tile scanning conversion process Follow the equations 6-3 and 6-4 This will provide correct offset_max in slice_header for parsing num_entry_point_offsets. https://bugzilla.gnome.org/show_bug.cgi?id=754024 Signed-off-by: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Created attachment 310010 [details] [review] codecparsers: h265: Avoid floating point arithmetic
(In reply to Sebastian Dröge (slomo) from comment #12) > Or just do > > (sps->pic_height_in_luma_samples + CtbSizeY - 1) / CtbSizeY right,, :), thanks.. > > I'll merge that patch for now as is, as it just moves the code around > anyway. But please provide a patch for later that gets rid of the floating > point arithmetic :) patch attached...