| Message ID | 20190905131255.10150-2-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] media: uapi: h264: clarify dec_ref_pic_marking_bit_size fields | expand |
On 9/5/19 3:12 PM, Philipp Zabel wrote: > Since pic_order_cnt_bit_size is not a syntax element itself, explicitly > state that it is the total size in bits of the pic_order_cnt_lsb, > delta_pic_order_cnt_bottom, delta_pic_order_cnt[0], and > delta_pic_order_cnt[1] syntax elements contained in the slice. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > index c281bc7ed1b3..08b49b2bbfa8 100644 > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > @@ -1799,7 +1799,8 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > - Size in bits of the dec_ref_pic_marking() syntax element. > * - __u32 > - ``pic_order_cnt_bit_size`` > - - > + - Size in bits of the pic_order_cnt_lsb, delta_pic_order_cnt_bottom, > + delta_pic_order_cnt[0], and delta_pic_order_cnt[1] syntax elements. It's a little bit ambiguous: this field contains the size in bits of all these fields combined, right? Not the size in bits of each field separately. I.e. if these fields are all 8 bits, then pic_order_cnt_bit_size is 4*8 and not just 8. I think this should be rephrased. Regards, Hans > * - __u8 > - ``cabac_init_idc`` > - >
Hi Hans, On Thu, 2019-09-26 at 15:23 +0200, Hans Verkuil wrote: > On 9/5/19 3:12 PM, Philipp Zabel wrote: > > Since pic_order_cnt_bit_size is not a syntax element itself, explicitly > > state that it is the total size in bits of the pic_order_cnt_lsb, > > delta_pic_order_cnt_bottom, delta_pic_order_cnt[0], and > > delta_pic_order_cnt[1] syntax elements contained in the slice. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > index c281bc7ed1b3..08b49b2bbfa8 100644 > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > @@ -1799,7 +1799,8 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > - Size in bits of the dec_ref_pic_marking() syntax element. > > * - __u32 > > - ``pic_order_cnt_bit_size`` > > - - > > + - Size in bits of the pic_order_cnt_lsb, delta_pic_order_cnt_bottom, > > + delta_pic_order_cnt[0], and delta_pic_order_cnt[1] syntax elements. > > It's a little bit ambiguous: this field contains the size in bits of all these > fields combined, right? Not the size in bits of each field separately. Yes. > I.e. if these fields are all 8 bits, then pic_order_cnt_bit_size is 4*8 and > not just 8. The size of pic_order_cnt_lsb is determined by another field's value (log2_max_pic_order_cnt_lsb_minus4), and the other three are exponential-Golomb coded, so each can be of different length (including zero). > I think this should be rephrased. Ok, how about: "Combined size in bits of the picture order count related syntax elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom, delta_pic_order_cnt[0], and delta_pic_order_cnt[1]." Actually, there's either pic_order_cnt_lsb + (optionally) delta_pic_order_cnt_bottom present, or delta_pic_order_cnt[0] + (optionally) delta_pic_order_cnt[1], never all four. Describing that in the uapi docs seemed overly verbose, though. The relevant part in the slice_header() syntax spec looks like this: if (pic_order_cnt_type == 0) { pic_order_cnt_lsb if (bottom_field_pic_order_in_frame_present_flag && !field_pic_flag) delta_pic_order_cnt_bottom } if (pic_order_cnt_type == 1 && !delta_pic_order_always_zero_flag) { delta_pic_order_cnt[0] if (bottom_field_pic_order_in_frame_present_flag && !field_pic_flag) delta_pic_order_cnt[1] } pic_order_cnt_bit_size is supposed to be the size in bits of this whole block. pic_order_cnt_type and log2_max_pic_order_cnt_lsb_minus4 are from the SPS header, bottom_field_pic_order_in_frame_present_flag is from the PPS header, and field_pic_flag is contained earlier in the slice header. regards Philipp
On 9/26/19 4:09 PM, Philipp Zabel wrote: > Hi Hans, > > On Thu, 2019-09-26 at 15:23 +0200, Hans Verkuil wrote: >> On 9/5/19 3:12 PM, Philipp Zabel wrote: >>> Since pic_order_cnt_bit_size is not a syntax element itself, explicitly >>> state that it is the total size in bits of the pic_order_cnt_lsb, >>> delta_pic_order_cnt_bottom, delta_pic_order_cnt[0], and >>> delta_pic_order_cnt[1] syntax elements contained in the slice. >>> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >>> --- >>> Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst >>> index c281bc7ed1b3..08b49b2bbfa8 100644 >>> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst >>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst >>> @@ -1799,7 +1799,8 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - >>> - Size in bits of the dec_ref_pic_marking() syntax element. >>> * - __u32 >>> - ``pic_order_cnt_bit_size`` >>> - - >>> + - Size in bits of the pic_order_cnt_lsb, delta_pic_order_cnt_bottom, >>> + delta_pic_order_cnt[0], and delta_pic_order_cnt[1] syntax elements. >> >> It's a little bit ambiguous: this field contains the size in bits of all these >> fields combined, right? Not the size in bits of each field separately. > > Yes. > >> I.e. if these fields are all 8 bits, then pic_order_cnt_bit_size is 4*8 and >> not just 8. > > The size of pic_order_cnt_lsb is determined by another field's value > (log2_max_pic_order_cnt_lsb_minus4), and the other three are > exponential-Golomb coded, so each can be of different length (including > zero). > >> I think this should be rephrased. > > Ok, how about: > > "Combined size in bits of the picture order count related syntax > elements: pic_order_cnt_lsb, delta_pic_order_cnt_bottom, > delta_pic_order_cnt[0], and delta_pic_order_cnt[1]." That's better. > > Actually, there's either pic_order_cnt_lsb + (optionally) > delta_pic_order_cnt_bottom present, or delta_pic_order_cnt[0] + > (optionally) delta_pic_order_cnt[1], never all four. Describing that in > the uapi docs seemed overly verbose, though. I agree. Regards, Hans The relevant part in the > slice_header() syntax spec looks like this: > > if (pic_order_cnt_type == 0) { > pic_order_cnt_lsb > if (bottom_field_pic_order_in_frame_present_flag && !field_pic_flag) > delta_pic_order_cnt_bottom > } > if (pic_order_cnt_type == 1 && !delta_pic_order_always_zero_flag) { > delta_pic_order_cnt[0] > if (bottom_field_pic_order_in_frame_present_flag && !field_pic_flag) > delta_pic_order_cnt[1] > } > > pic_order_cnt_bit_size is supposed to be the size in bits of this whole > block. pic_order_cnt_type and log2_max_pic_order_cnt_lsb_minus4 are from > the SPS header, bottom_field_pic_order_in_frame_present_flag is from the > PPS header, and field_pic_flag is contained earlier in the slice header. > > regards > Philipp >
diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst index c281bc7ed1b3..08b49b2bbfa8 100644 --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst @@ -1799,7 +1799,8 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - - Size in bits of the dec_ref_pic_marking() syntax element. * - __u32 - ``pic_order_cnt_bit_size`` - - + - Size in bits of the pic_order_cnt_lsb, delta_pic_order_cnt_bottom, + delta_pic_order_cnt[0], and delta_pic_order_cnt[1] syntax elements. * - __u8 - ``cabac_init_idc`` -
Since pic_order_cnt_bit_size is not a syntax element itself, explicitly state that it is the total size in bits of the pic_order_cnt_lsb, delta_pic_order_cnt_bottom, delta_pic_order_cnt[0], and delta_pic_order_cnt[1] syntax elements contained in the slice. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)