| Message ID | 20190816160132.7352-3-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: hantro: Add support for H264 decoding | expand |
Hi, On Fri 16 Aug 19, 13:01, Ezequiel Garcia wrote: > The V4L2_PIX_FMT_H264_SLICE_RAW name was originally suggested > because the pixel format would represent H264 slices without any > start code. > > However, as we will now introduce a start code menu control, > give the pixel format a more meaningful name, while it's > still early enough to do so. I definitely agree that SLICE_RAW is not the suffix we are looking for, but I'm not sure that _SLICE is self-describing given that we can operate either per-frame or per-slice, and _SLICE sort of implies the latter. Also, VP8 uses _FRAME to clearly indicate that it operates per-frame. In addition, the _SLICE suffix is used by MPEG-2 in the stable API. Since we certainly want MPEG-2 to allow per-slice and per-frame decoding as well as H.264 and that the _SLICE format is specified to be the broken "concatenated slices" that cedrus expects, we probably want to use another suffix. This way, we could deprecated MPEG2_SLICE and introduce a new format for MPEG-2 that would have consistent naming with the other mpeg formats. One suggestion I had was to call it H264_PARSED (and apply this to MPEG-2 and HEVC when similar controls to H.264 are set in place for them). I think Hans had another suggestion for the name but I don't recall what it was at this point. Either way, if this has to be some debate, we could perhaps take it off your series and stay with SLICE_RAW for now, as long as we do rename it before making the API stable. What do you think? Cheers, Paul > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > Tested-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > Changes in v7: > * None. > Changes in v6: > * None. > Changes in v5: > * None. > Changes in v4: > * New patch. > --- > Documentation/media/uapi/v4l/pixfmt-compressed.rst | 4 ++-- > drivers/media/v4l2-core/v4l2-ioctl.c | 2 +- > drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +- > drivers/staging/media/sunxi/cedrus/cedrus_video.c | 6 +++--- > include/media/h264-ctrls.h | 2 +- > 5 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst b/Documentation/media/uapi/v4l/pixfmt-compressed.rst > index f52a7b67023d..9b65473a2288 100644 > --- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst > +++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst > @@ -52,9 +52,9 @@ Compressed Formats > - ``V4L2_PIX_FMT_H264_MVC`` > - 'M264' > - H264 MVC video elementary stream. > - * .. _V4L2-PIX-FMT-H264-SLICE-RAW: > + * .. _V4L2-PIX-FMT-H264-SLICE: > > - - ``V4L2_PIX_FMT_H264_SLICE_RAW`` > + - ``V4L2_PIX_FMT_H264_SLICE`` > - 'S264' > - H264 parsed slice data, without the start code and as > extracted from the H264 bitstream. This format is adapted for > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index bb5b4926538a..39f10621c91b 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -1343,7 +1343,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) > case V4L2_PIX_FMT_H264: descr = "H.264"; break; > case V4L2_PIX_FMT_H264_NO_SC: descr = "H.264 (No Start Codes)"; break; > case V4L2_PIX_FMT_H264_MVC: descr = "H.264 MVC"; break; > - case V4L2_PIX_FMT_H264_SLICE_RAW: descr = "H.264 Parsed Slice Data"; break; > + case V4L2_PIX_FMT_H264_SLICE: descr = "H.264 Parsed Slice Data"; break; > case V4L2_PIX_FMT_H263: descr = "H.263"; break; > case V4L2_PIX_FMT_MPEG1: descr = "MPEG-1 ES"; break; > case V4L2_PIX_FMT_MPEG2: descr = "MPEG-2 ES"; break; > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > index bdad87eb9d79..56ca4c9ad01c 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > @@ -46,7 +46,7 @@ void cedrus_device_run(void *priv) > V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION); > break; > > - case V4L2_PIX_FMT_H264_SLICE_RAW: > + case V4L2_PIX_FMT_H264_SLICE: > run.h264.decode_params = cedrus_find_control_data(ctx, > V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS); > run.h264.pps = cedrus_find_control_data(ctx, > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > index 681dfe3367a6..eeee3efd247b 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > @@ -38,7 +38,7 @@ static struct cedrus_format cedrus_formats[] = { > .directions = CEDRUS_DECODE_SRC, > }, > { > - .pixelformat = V4L2_PIX_FMT_H264_SLICE_RAW, > + .pixelformat = V4L2_PIX_FMT_H264_SLICE, > .directions = CEDRUS_DECODE_SRC, > }, > { > @@ -104,7 +104,7 @@ static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt) > > switch (pix_fmt->pixelformat) { > case V4L2_PIX_FMT_MPEG2_SLICE: > - case V4L2_PIX_FMT_H264_SLICE_RAW: > + case V4L2_PIX_FMT_H264_SLICE: > /* Zero bytes per line for encoded source. */ > bytesperline = 0; > > @@ -449,7 +449,7 @@ static int cedrus_start_streaming(struct vb2_queue *vq, unsigned int count) > ctx->current_codec = CEDRUS_CODEC_MPEG2; > break; > > - case V4L2_PIX_FMT_H264_SLICE_RAW: > + case V4L2_PIX_FMT_H264_SLICE: > ctx->current_codec = CEDRUS_CODEC_H264; > break; > > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h > index e1404d78d6ff..6160a69c0143 100644 > --- a/include/media/h264-ctrls.h > +++ b/include/media/h264-ctrls.h > @@ -14,7 +14,7 @@ > #include <linux/videodev2.h> > > /* Our pixel format isn't stable at the moment */ > -#define V4L2_PIX_FMT_H264_SLICE_RAW v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */ > +#define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */ > > /* > * This is put insanely high to avoid conflicting with controls that > -- > 2.22.0 >
On Mon, 2019-08-19 at 14:41 +0200, Paul Kocialkowski wrote: > Hi, > > On Fri 16 Aug 19, 13:01, Ezequiel Garcia wrote: > > The V4L2_PIX_FMT_H264_SLICE_RAW name was originally suggested > > because the pixel format would represent H264 slices without any > > start code. > > > > However, as we will now introduce a start code menu control, > > give the pixel format a more meaningful name, while it's > > still early enough to do so. > > I definitely agree that SLICE_RAW is not the suffix we are looking for, but I'm > not sure that _SLICE is self-describing given that we can operate either > per-frame or per-slice, and _SLICE sort of implies the latter. This is not entirely so, per-frame or per-slice mode refer to the granularity of the stateless API, the pixel format is still composed of H264 NALU slices. As per the documentation, the decode_mode and the start_code are modifiers of the pixel format. As long as this is properly speced, the V4L2_PIX_FMT_H264_SLICE looks pretty OK to me. > Also, VP8 uses > _FRAME to clearly indicate that it operates per-frame. > This is because VP8 doesn't have any concept of slices, the encoded unit is a video frame. > In addition, the _SLICE suffix is used by MPEG-2 in the stable API. Since we > certainly want MPEG-2 to allow per-slice and per-frame decoding as well as > H.264 and that the _SLICE format is specified to be the broken "concatenated > slices" that cedrus expects, we probably want to use another suffix. This way, > we could deprecated MPEG2_SLICE and introduce a new format for MPEG-2 that would > have consistent naming with the other mpeg formats. > > One suggestion I had was to call it H264_PARSED (and apply this to MPEG-2 and > HEVC when similar controls to H.264 are set in place for them). I think Hans had > another suggestion for the name but I don't recall what it was at this point. > > Either way, if this has to be some debate, we could perhaps take it off your > series and stay with SLICE_RAW for now, as long as we do rename it before making > the API stable. > > What do you think? > With the new pixel format modifiers (decode_mode and start_code), the _RAW suffix now has no meaning. Now, we could name it _PARSED or _SLICE. As long as this is properly documented (as it is now), that'd be fine. Now, to be completely honest, this discussion sounds like bikeshedding to me. Thanks, Eze
On 8/19/19 2:41 PM, Paul Kocialkowski wrote: > Hi, > > On Fri 16 Aug 19, 13:01, Ezequiel Garcia wrote: >> The V4L2_PIX_FMT_H264_SLICE_RAW name was originally suggested >> because the pixel format would represent H264 slices without any >> start code. >> >> However, as we will now introduce a start code menu control, >> give the pixel format a more meaningful name, while it's >> still early enough to do so. > > I definitely agree that SLICE_RAW is not the suffix we are looking for, but I'm > not sure that _SLICE is self-describing given that we can operate either > per-frame or per-slice, and _SLICE sort of implies the latter. Also, VP8 uses > _FRAME to clearly indicate that it operates per-frame. Well, VP8 doesn't support slices at all. > > In addition, the _SLICE suffix is used by MPEG-2 in the stable API. Since we Regarding MPEG-2: while it has a concept of slices, it is my understanding that you never process slices separately, but only full pictures. I may be wrong here. > certainly want MPEG-2 to allow per-slice and per-frame decoding as well as > H.264 and that the _SLICE format is specified to be the broken "concatenated > slices" that cedrus expects, we probably want to use another suffix. This way, > we could deprecated MPEG2_SLICE and introduce a new format for MPEG-2 that would > have consistent naming with the other mpeg formats. I actually think that H264_SLICE is a decent name. I'm less sure about MPEG2_SLICE since I am not sure if it means the same as a H264 slice. > > One suggestion I had was to call it H264_PARSED (and apply this to MPEG-2 and > HEVC when similar controls to H.264 are set in place for them). I think Hans had > another suggestion for the name but I don't recall what it was at this point. I can't remember it either. In any case, I'm not that keen on _PARSED. I think for H264 and HEVC the _SLICE suffix is good enough. Regards, Hans > > Either way, if this has to be some debate, we could perhaps take it off your > series and stay with SLICE_RAW for now, as long as we do rename it before making > the API stable. > > What do you think? > > Cheers, > > Paul > >> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >> Tested-by: Philipp Zabel <p.zabel@pengutronix.de> >> --- >> Changes in v7: >> * None. >> Changes in v6: >> * None. >> Changes in v5: >> * None. >> Changes in v4: >> * New patch. >> --- >> Documentation/media/uapi/v4l/pixfmt-compressed.rst | 4 ++-- >> drivers/media/v4l2-core/v4l2-ioctl.c | 2 +- >> drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +- >> drivers/staging/media/sunxi/cedrus/cedrus_video.c | 6 +++--- >> include/media/h264-ctrls.h | 2 +- >> 5 files changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst b/Documentation/media/uapi/v4l/pixfmt-compressed.rst >> index f52a7b67023d..9b65473a2288 100644 >> --- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst >> +++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst >> @@ -52,9 +52,9 @@ Compressed Formats >> - ``V4L2_PIX_FMT_H264_MVC`` >> - 'M264' >> - H264 MVC video elementary stream. >> - * .. _V4L2-PIX-FMT-H264-SLICE-RAW: >> + * .. _V4L2-PIX-FMT-H264-SLICE: >> >> - - ``V4L2_PIX_FMT_H264_SLICE_RAW`` >> + - ``V4L2_PIX_FMT_H264_SLICE`` >> - 'S264' >> - H264 parsed slice data, without the start code and as >> extracted from the H264 bitstream. This format is adapted for >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >> index bb5b4926538a..39f10621c91b 100644 >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >> @@ -1343,7 +1343,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) >> case V4L2_PIX_FMT_H264: descr = "H.264"; break; >> case V4L2_PIX_FMT_H264_NO_SC: descr = "H.264 (No Start Codes)"; break; >> case V4L2_PIX_FMT_H264_MVC: descr = "H.264 MVC"; break; >> - case V4L2_PIX_FMT_H264_SLICE_RAW: descr = "H.264 Parsed Slice Data"; break; >> + case V4L2_PIX_FMT_H264_SLICE: descr = "H.264 Parsed Slice Data"; break; >> case V4L2_PIX_FMT_H263: descr = "H.263"; break; >> case V4L2_PIX_FMT_MPEG1: descr = "MPEG-1 ES"; break; >> case V4L2_PIX_FMT_MPEG2: descr = "MPEG-2 ES"; break; >> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >> index bdad87eb9d79..56ca4c9ad01c 100644 >> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >> @@ -46,7 +46,7 @@ void cedrus_device_run(void *priv) >> V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION); >> break; >> >> - case V4L2_PIX_FMT_H264_SLICE_RAW: >> + case V4L2_PIX_FMT_H264_SLICE: >> run.h264.decode_params = cedrus_find_control_data(ctx, >> V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS); >> run.h264.pps = cedrus_find_control_data(ctx, >> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c >> index 681dfe3367a6..eeee3efd247b 100644 >> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c >> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c >> @@ -38,7 +38,7 @@ static struct cedrus_format cedrus_formats[] = { >> .directions = CEDRUS_DECODE_SRC, >> }, >> { >> - .pixelformat = V4L2_PIX_FMT_H264_SLICE_RAW, >> + .pixelformat = V4L2_PIX_FMT_H264_SLICE, >> .directions = CEDRUS_DECODE_SRC, >> }, >> { >> @@ -104,7 +104,7 @@ static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt) >> >> switch (pix_fmt->pixelformat) { >> case V4L2_PIX_FMT_MPEG2_SLICE: >> - case V4L2_PIX_FMT_H264_SLICE_RAW: >> + case V4L2_PIX_FMT_H264_SLICE: >> /* Zero bytes per line for encoded source. */ >> bytesperline = 0; >> >> @@ -449,7 +449,7 @@ static int cedrus_start_streaming(struct vb2_queue *vq, unsigned int count) >> ctx->current_codec = CEDRUS_CODEC_MPEG2; >> break; >> >> - case V4L2_PIX_FMT_H264_SLICE_RAW: >> + case V4L2_PIX_FMT_H264_SLICE: >> ctx->current_codec = CEDRUS_CODEC_H264; >> break; >> >> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h >> index e1404d78d6ff..6160a69c0143 100644 >> --- a/include/media/h264-ctrls.h >> +++ b/include/media/h264-ctrls.h >> @@ -14,7 +14,7 @@ >> #include <linux/videodev2.h> >> >> /* Our pixel format isn't stable at the moment */ >> -#define V4L2_PIX_FMT_H264_SLICE_RAW v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */ >> +#define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */ >> >> /* >> * This is put insanely high to avoid conflicting with controls that >> -- >> 2.22.0 >> >
Hi, On Mon 19 Aug 19, 17:53, Hans Verkuil wrote: > On 8/19/19 2:41 PM, Paul Kocialkowski wrote: > > Hi, > > > > On Fri 16 Aug 19, 13:01, Ezequiel Garcia wrote: > >> The V4L2_PIX_FMT_H264_SLICE_RAW name was originally suggested > >> because the pixel format would represent H264 slices without any > >> start code. > >> > >> However, as we will now introduce a start code menu control, > >> give the pixel format a more meaningful name, while it's > >> still early enough to do so. > > > > I definitely agree that SLICE_RAW is not the suffix we are looking for, but I'm > > not sure that _SLICE is self-describing given that we can operate either > > per-frame or per-slice, and _SLICE sort of implies the latter. Also, VP8 uses > > _FRAME to clearly indicate that it operates per-frame. > > Well, VP8 doesn't support slices at all. > > > > > In addition, the _SLICE suffix is used by MPEG-2 in the stable API. Since we > > Regarding MPEG-2: while it has a concept of slices, it is my understanding > that you never process slices separately, but only full pictures. I may be > wrong here. I don't think that is the case since ffmpeg clearly implements decoding on a per-slice basis (mpeg_decode_slice). Information is also passed on a per-slice basis to VAAPI (vaapi_mpeg2_decode_slice) with a distinct data buffer and slice parameter buffer for each slice. Among other things, it contains the vertical and horizontal positions for the slice, which we can set in the hardware. > > certainly want MPEG-2 to allow per-slice and per-frame decoding as well as > > H.264 and that the _SLICE format is specified to be the broken "concatenated > > slices" that cedrus expects, we probably want to use another suffix. This way, > > we could deprecated MPEG2_SLICE and introduce a new format for MPEG-2 that would > > have consistent naming with the other mpeg formats. > > I actually think that H264_SLICE is a decent name. > > I'm less sure about MPEG2_SLICE since I am not sure if it means the same as > a H264 slice. The main problem I see is that we have already specified MPEG2_SLICE in a way that is incompatible with the future improvments we want to bring to the API: " The output buffer must contain the appropriate number of macroblocks to decode a full corresponding frame to the matching capture buffer." So I only see two possibilities: either we decide to change the specification of the pixel format and we can keep using the _SLICE suffix, either we need to introduce a new pixel format with another suffix, which should also be reflected on other MPEG formats for consistency. Then we can deprecate MPEG2_SLICE and have drivers stop using it. What do you think? Cheers, Paul > > One suggestion I had was to call it H264_PARSED (and apply this to MPEG-2 and > > HEVC when similar controls to H.264 are set in place for them). I think Hans had > > another suggestion for the name but I don't recall what it was at this point. > > I can't remember it either. In any case, I'm not that keen on _PARSED. > > I think for H264 and HEVC the _SLICE suffix is good enough. > > Regards, > > Hans > > > > > Either way, if this has to be some debate, we could perhaps take it off your > > series and stay with SLICE_RAW for now, as long as we do rename it before making > > the API stable. > > > > What do you think? > > > > Cheers, > > > > Paul > > > >> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > >> Tested-by: Philipp Zabel <p.zabel@pengutronix.de> > >> --- > >> Changes in v7: > >> * None. > >> Changes in v6: > >> * None. > >> Changes in v5: > >> * None. > >> Changes in v4: > >> * New patch. > >> --- > >> Documentation/media/uapi/v4l/pixfmt-compressed.rst | 4 ++-- > >> drivers/media/v4l2-core/v4l2-ioctl.c | 2 +- > >> drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +- > >> drivers/staging/media/sunxi/cedrus/cedrus_video.c | 6 +++--- > >> include/media/h264-ctrls.h | 2 +- > >> 5 files changed, 8 insertions(+), 8 deletions(-) > >> > >> diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst b/Documentation/media/uapi/v4l/pixfmt-compressed.rst > >> index f52a7b67023d..9b65473a2288 100644 > >> --- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst > >> +++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst > >> @@ -52,9 +52,9 @@ Compressed Formats > >> - ``V4L2_PIX_FMT_H264_MVC`` > >> - 'M264' > >> - H264 MVC video elementary stream. > >> - * .. _V4L2-PIX-FMT-H264-SLICE-RAW: > >> + * .. _V4L2-PIX-FMT-H264-SLICE: > >> > >> - - ``V4L2_PIX_FMT_H264_SLICE_RAW`` > >> + - ``V4L2_PIX_FMT_H264_SLICE`` > >> - 'S264' > >> - H264 parsed slice data, without the start code and as > >> extracted from the H264 bitstream. This format is adapted for > >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > >> index bb5b4926538a..39f10621c91b 100644 > >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c > >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > >> @@ -1343,7 +1343,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) > >> case V4L2_PIX_FMT_H264: descr = "H.264"; break; > >> case V4L2_PIX_FMT_H264_NO_SC: descr = "H.264 (No Start Codes)"; break; > >> case V4L2_PIX_FMT_H264_MVC: descr = "H.264 MVC"; break; > >> - case V4L2_PIX_FMT_H264_SLICE_RAW: descr = "H.264 Parsed Slice Data"; break; > >> + case V4L2_PIX_FMT_H264_SLICE: descr = "H.264 Parsed Slice Data"; break; > >> case V4L2_PIX_FMT_H263: descr = "H.263"; break; > >> case V4L2_PIX_FMT_MPEG1: descr = "MPEG-1 ES"; break; > >> case V4L2_PIX_FMT_MPEG2: descr = "MPEG-2 ES"; break; > >> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > >> index bdad87eb9d79..56ca4c9ad01c 100644 > >> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > >> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > >> @@ -46,7 +46,7 @@ void cedrus_device_run(void *priv) > >> V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION); > >> break; > >> > >> - case V4L2_PIX_FMT_H264_SLICE_RAW: > >> + case V4L2_PIX_FMT_H264_SLICE: > >> run.h264.decode_params = cedrus_find_control_data(ctx, > >> V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS); > >> run.h264.pps = cedrus_find_control_data(ctx, > >> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > >> index 681dfe3367a6..eeee3efd247b 100644 > >> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c > >> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > >> @@ -38,7 +38,7 @@ static struct cedrus_format cedrus_formats[] = { > >> .directions = CEDRUS_DECODE_SRC, > >> }, > >> { > >> - .pixelformat = V4L2_PIX_FMT_H264_SLICE_RAW, > >> + .pixelformat = V4L2_PIX_FMT_H264_SLICE, > >> .directions = CEDRUS_DECODE_SRC, > >> }, > >> { > >> @@ -104,7 +104,7 @@ static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt) > >> > >> switch (pix_fmt->pixelformat) { > >> case V4L2_PIX_FMT_MPEG2_SLICE: > >> - case V4L2_PIX_FMT_H264_SLICE_RAW: > >> + case V4L2_PIX_FMT_H264_SLICE: > >> /* Zero bytes per line for encoded source. */ > >> bytesperline = 0; > >> > >> @@ -449,7 +449,7 @@ static int cedrus_start_streaming(struct vb2_queue *vq, unsigned int count) > >> ctx->current_codec = CEDRUS_CODEC_MPEG2; > >> break; > >> > >> - case V4L2_PIX_FMT_H264_SLICE_RAW: > >> + case V4L2_PIX_FMT_H264_SLICE: > >> ctx->current_codec = CEDRUS_CODEC_H264; > >> break; > >> > >> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h > >> index e1404d78d6ff..6160a69c0143 100644 > >> --- a/include/media/h264-ctrls.h > >> +++ b/include/media/h264-ctrls.h > >> @@ -14,7 +14,7 @@ > >> #include <linux/videodev2.h> > >> > >> /* Our pixel format isn't stable at the moment */ > >> -#define V4L2_PIX_FMT_H264_SLICE_RAW v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */ > >> +#define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */ > >> > >> /* > >> * This is put insanely high to avoid conflicting with controls that > >> -- > >> 2.22.0 > >> > > >
On 8/22/19 1:54 PM, Paul Kocialkowski wrote: > Hi, > > On Mon 19 Aug 19, 17:53, Hans Verkuil wrote: >> On 8/19/19 2:41 PM, Paul Kocialkowski wrote: >>> Hi, >>> >>> On Fri 16 Aug 19, 13:01, Ezequiel Garcia wrote: >>>> The V4L2_PIX_FMT_H264_SLICE_RAW name was originally suggested >>>> because the pixel format would represent H264 slices without any >>>> start code. >>>> >>>> However, as we will now introduce a start code menu control, >>>> give the pixel format a more meaningful name, while it's >>>> still early enough to do so. >>> >>> I definitely agree that SLICE_RAW is not the suffix we are looking for, but I'm >>> not sure that _SLICE is self-describing given that we can operate either >>> per-frame or per-slice, and _SLICE sort of implies the latter. Also, VP8 uses >>> _FRAME to clearly indicate that it operates per-frame. >> >> Well, VP8 doesn't support slices at all. >> >>> >>> In addition, the _SLICE suffix is used by MPEG-2 in the stable API. Since we >> >> Regarding MPEG-2: while it has a concept of slices, it is my understanding >> that you never process slices separately, but only full pictures. I may be >> wrong here. > > I don't think that is the case since ffmpeg clearly implements decoding on a > per-slice basis (mpeg_decode_slice). > > Information is also passed on a per-slice basis to VAAPI > (vaapi_mpeg2_decode_slice) with a distinct data buffer and slice parameter > buffer for each slice. Among other things, it contains the vertical and > horizontal positions for the slice, which we can set in the hardware. > >>> certainly want MPEG-2 to allow per-slice and per-frame decoding as well as >>> H.264 and that the _SLICE format is specified to be the broken "concatenated >>> slices" that cedrus expects, we probably want to use another suffix. This way, >>> we could deprecated MPEG2_SLICE and introduce a new format for MPEG-2 that would >>> have consistent naming with the other mpeg formats. >> >> I actually think that H264_SLICE is a decent name. >> >> I'm less sure about MPEG2_SLICE since I am not sure if it means the same as >> a H264 slice. > > The main problem I see is that we have already specified MPEG2_SLICE in a way > that is incompatible with the future improvments we want to bring to the API: > " The output buffer must contain the appropriate number of macroblocks to > decode a full corresponding frame to the matching capture buffer." > > So I only see two possibilities: either we decide to change the specification > of the pixel format and we can keep using the _SLICE suffix, either we need to > introduce a new pixel format with another suffix, which should also be reflected > on other MPEG formats for consistency. Then we can deprecate MPEG2_SLICE and > have drivers stop using it. > > What do you think? I'd change the specification of the pixel format. So MPEG2_SLICE now supports multiple slices if the hardware supports it as well. We would need an MPEG2_DECODING_MODE control as well, that currently would read FRAME based only. Regards, Hans > > Cheers, > > Paul > >>> One suggestion I had was to call it H264_PARSED (and apply this to MPEG-2 and >>> HEVC when similar controls to H.264 are set in place for them). I think Hans had >>> another suggestion for the name but I don't recall what it was at this point. >> >> I can't remember it either. In any case, I'm not that keen on _PARSED. >> >> I think for H264 and HEVC the _SLICE suffix is good enough. >> >> Regards, >> >> Hans >> >>> >>> Either way, if this has to be some debate, we could perhaps take it off your >>> series and stay with SLICE_RAW for now, as long as we do rename it before making >>> the API stable. >>> >>> What do you think? >>> >>> Cheers, >>> >>> Paul >>> >>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >>>> Tested-by: Philipp Zabel <p.zabel@pengutronix.de> >>>> --- >>>> Changes in v7: >>>> * None. >>>> Changes in v6: >>>> * None. >>>> Changes in v5: >>>> * None. >>>> Changes in v4: >>>> * New patch. >>>> --- >>>> Documentation/media/uapi/v4l/pixfmt-compressed.rst | 4 ++-- >>>> drivers/media/v4l2-core/v4l2-ioctl.c | 2 +- >>>> drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 +- >>>> drivers/staging/media/sunxi/cedrus/cedrus_video.c | 6 +++--- >>>> include/media/h264-ctrls.h | 2 +- >>>> 5 files changed, 8 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst b/Documentation/media/uapi/v4l/pixfmt-compressed.rst >>>> index f52a7b67023d..9b65473a2288 100644 >>>> --- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst >>>> +++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst >>>> @@ -52,9 +52,9 @@ Compressed Formats >>>> - ``V4L2_PIX_FMT_H264_MVC`` >>>> - 'M264' >>>> - H264 MVC video elementary stream. >>>> - * .. _V4L2-PIX-FMT-H264-SLICE-RAW: >>>> + * .. _V4L2-PIX-FMT-H264-SLICE: >>>> >>>> - - ``V4L2_PIX_FMT_H264_SLICE_RAW`` >>>> + - ``V4L2_PIX_FMT_H264_SLICE`` >>>> - 'S264' >>>> - H264 parsed slice data, without the start code and as >>>> extracted from the H264 bitstream. This format is adapted for >>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >>>> index bb5b4926538a..39f10621c91b 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >>>> @@ -1343,7 +1343,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) >>>> case V4L2_PIX_FMT_H264: descr = "H.264"; break; >>>> case V4L2_PIX_FMT_H264_NO_SC: descr = "H.264 (No Start Codes)"; break; >>>> case V4L2_PIX_FMT_H264_MVC: descr = "H.264 MVC"; break; >>>> - case V4L2_PIX_FMT_H264_SLICE_RAW: descr = "H.264 Parsed Slice Data"; break; >>>> + case V4L2_PIX_FMT_H264_SLICE: descr = "H.264 Parsed Slice Data"; break; >>>> case V4L2_PIX_FMT_H263: descr = "H.263"; break; >>>> case V4L2_PIX_FMT_MPEG1: descr = "MPEG-1 ES"; break; >>>> case V4L2_PIX_FMT_MPEG2: descr = "MPEG-2 ES"; break; >>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >>>> index bdad87eb9d79..56ca4c9ad01c 100644 >>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >>>> @@ -46,7 +46,7 @@ void cedrus_device_run(void *priv) >>>> V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION); >>>> break; >>>> >>>> - case V4L2_PIX_FMT_H264_SLICE_RAW: >>>> + case V4L2_PIX_FMT_H264_SLICE: >>>> run.h264.decode_params = cedrus_find_control_data(ctx, >>>> V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS); >>>> run.h264.pps = cedrus_find_control_data(ctx, >>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c >>>> index 681dfe3367a6..eeee3efd247b 100644 >>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c >>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c >>>> @@ -38,7 +38,7 @@ static struct cedrus_format cedrus_formats[] = { >>>> .directions = CEDRUS_DECODE_SRC, >>>> }, >>>> { >>>> - .pixelformat = V4L2_PIX_FMT_H264_SLICE_RAW, >>>> + .pixelformat = V4L2_PIX_FMT_H264_SLICE, >>>> .directions = CEDRUS_DECODE_SRC, >>>> }, >>>> { >>>> @@ -104,7 +104,7 @@ static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt) >>>> >>>> switch (pix_fmt->pixelformat) { >>>> case V4L2_PIX_FMT_MPEG2_SLICE: >>>> - case V4L2_PIX_FMT_H264_SLICE_RAW: >>>> + case V4L2_PIX_FMT_H264_SLICE: >>>> /* Zero bytes per line for encoded source. */ >>>> bytesperline = 0; >>>> >>>> @@ -449,7 +449,7 @@ static int cedrus_start_streaming(struct vb2_queue *vq, unsigned int count) >>>> ctx->current_codec = CEDRUS_CODEC_MPEG2; >>>> break; >>>> >>>> - case V4L2_PIX_FMT_H264_SLICE_RAW: >>>> + case V4L2_PIX_FMT_H264_SLICE: >>>> ctx->current_codec = CEDRUS_CODEC_H264; >>>> break; >>>> >>>> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h >>>> index e1404d78d6ff..6160a69c0143 100644 >>>> --- a/include/media/h264-ctrls.h >>>> +++ b/include/media/h264-ctrls.h >>>> @@ -14,7 +14,7 @@ >>>> #include <linux/videodev2.h> >>>> >>>> /* Our pixel format isn't stable at the moment */ >>>> -#define V4L2_PIX_FMT_H264_SLICE_RAW v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */ >>>> +#define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */ >>>> >>>> /* >>>> * This is put insanely high to avoid conflicting with controls that >>>> -- >>>> 2.22.0 >>>> >>> >> >
On Thu, 2019-08-22 at 15:47 +0200, Hans Verkuil wrote: > On 8/22/19 1:54 PM, Paul Kocialkowski wrote: > > Hi, > > > > On Mon 19 Aug 19, 17:53, Hans Verkuil wrote: > > > On 8/19/19 2:41 PM, Paul Kocialkowski wrote: > > > > Hi, > > > > > > > > On Fri 16 Aug 19, 13:01, Ezequiel Garcia wrote: > > > > > The V4L2_PIX_FMT_H264_SLICE_RAW name was originally suggested > > > > > because the pixel format would represent H264 slices without any > > > > > start code. > > > > > > > > > > However, as we will now introduce a start code menu control, > > > > > give the pixel format a more meaningful name, while it's > > > > > still early enough to do so. > > > > > > > > I definitely agree that SLICE_RAW is not the suffix we are looking for, but I'm > > > > not sure that _SLICE is self-describing given that we can operate either > > > > per-frame or per-slice, and _SLICE sort of implies the latter. Also, VP8 uses > > > > _FRAME to clearly indicate that it operates per-frame. > > > > > > Well, VP8 doesn't support slices at all. > > > > > > > In addition, the _SLICE suffix is used by MPEG-2 in the stable API. Since we > > > > > > Regarding MPEG-2: while it has a concept of slices, it is my understanding > > > that you never process slices separately, but only full pictures. I may be > > > wrong here. > > > > I don't think that is the case since ffmpeg clearly implements decoding on a > > per-slice basis (mpeg_decode_slice). > > > > Information is also passed on a per-slice basis to VAAPI > > (vaapi_mpeg2_decode_slice) with a distinct data buffer and slice parameter > > buffer for each slice. Among other things, it contains the vertical and > > horizontal positions for the slice, which we can set in the hardware. > > > > > > certainly want MPEG-2 to allow per-slice and per-frame decoding as well as > > > > H.264 and that the _SLICE format is specified to be the broken "concatenated > > > > slices" that cedrus expects, we probably want to use another suffix. This way, > > > > we could deprecated MPEG2_SLICE and introduce a new format for MPEG-2 that would > > > > have consistent naming with the other mpeg formats. > > > > > > I actually think that H264_SLICE is a decent name. > > > > > > I'm less sure about MPEG2_SLICE since I am not sure if it means the same as > > > a H264 slice. > > > > The main problem I see is that we have already specified MPEG2_SLICE in a way > > that is incompatible with the future improvments we want to bring to the API: > > " The output buffer must contain the appropriate number of macroblocks to > > decode a full corresponding frame to the matching capture buffer." > > > > So I only see two possibilities: either we decide to change the specification > > of the pixel format and we can keep using the _SLICE suffix, either we need to > > introduce a new pixel format with another suffix, which should also be reflected > > on other MPEG formats for consistency. Then we can deprecate MPEG2_SLICE and > > have drivers stop using it. > > > > What do you think? > > I'd change the specification of the pixel format. So MPEG2_SLICE now supports > multiple slices if the hardware supports it as well. > > We would need an MPEG2_DECODING_MODE control as well, that currently would > read FRAME based only. > That's exactly what I was about to suggest. Regards, Ezequiel
Hi, On Thu 22 Aug 19, 11:38, Ezequiel Garcia wrote: > On Thu, 2019-08-22 at 15:47 +0200, Hans Verkuil wrote: > > On 8/22/19 1:54 PM, Paul Kocialkowski wrote: > > > Hi, > > > > > > On Mon 19 Aug 19, 17:53, Hans Verkuil wrote: > > > > On 8/19/19 2:41 PM, Paul Kocialkowski wrote: > > > > > Hi, > > > > > > > > > > On Fri 16 Aug 19, 13:01, Ezequiel Garcia wrote: > > > > > > The V4L2_PIX_FMT_H264_SLICE_RAW name was originally suggested > > > > > > because the pixel format would represent H264 slices without any > > > > > > start code. > > > > > > > > > > > > However, as we will now introduce a start code menu control, > > > > > > give the pixel format a more meaningful name, while it's > > > > > > still early enough to do so. > > > > > > > > > > I definitely agree that SLICE_RAW is not the suffix we are looking for, but I'm > > > > > not sure that _SLICE is self-describing given that we can operate either > > > > > per-frame or per-slice, and _SLICE sort of implies the latter. Also, VP8 uses > > > > > _FRAME to clearly indicate that it operates per-frame. > > > > > > > > Well, VP8 doesn't support slices at all. > > > > > > > > > In addition, the _SLICE suffix is used by MPEG-2 in the stable API. Since we > > > > > > > > Regarding MPEG-2: while it has a concept of slices, it is my understanding > > > > that you never process slices separately, but only full pictures. I may be > > > > wrong here. > > > > > > I don't think that is the case since ffmpeg clearly implements decoding on a > > > per-slice basis (mpeg_decode_slice). > > > > > > Information is also passed on a per-slice basis to VAAPI > > > (vaapi_mpeg2_decode_slice) with a distinct data buffer and slice parameter > > > buffer for each slice. Among other things, it contains the vertical and > > > horizontal positions for the slice, which we can set in the hardware. > > > > > > > > certainly want MPEG-2 to allow per-slice and per-frame decoding as well as > > > > > H.264 and that the _SLICE format is specified to be the broken "concatenated > > > > > slices" that cedrus expects, we probably want to use another suffix. This way, > > > > > we could deprecated MPEG2_SLICE and introduce a new format for MPEG-2 that would > > > > > have consistent naming with the other mpeg formats. > > > > > > > > I actually think that H264_SLICE is a decent name. > > > > > > > > I'm less sure about MPEG2_SLICE since I am not sure if it means the same as > > > > a H264 slice. > > > > > > The main problem I see is that we have already specified MPEG2_SLICE in a way > > > that is incompatible with the future improvments we want to bring to the API: > > > " The output buffer must contain the appropriate number of macroblocks to > > > decode a full corresponding frame to the matching capture buffer." > > > > > > So I only see two possibilities: either we decide to change the specification > > > of the pixel format and we can keep using the _SLICE suffix, either we need to > > > introduce a new pixel format with another suffix, which should also be reflected > > > on other MPEG formats for consistency. Then we can deprecate MPEG2_SLICE and > > > have drivers stop using it. > > > > > > What do you think? > > > > I'd change the specification of the pixel format. So MPEG2_SLICE now supports > > multiple slices if the hardware supports it as well. > > > > We would need an MPEG2_DECODING_MODE control as well, that currently would > > read FRAME based only. > > > > That's exactly what I was about to suggest. Sounds perfect then! I have started looking at the start-code situation to see if it shares similarities with H.264, but did not go far enough to reach any conclusion on that aspect. Cheers, Paul
diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst b/Documentation/media/uapi/v4l/pixfmt-compressed.rst index f52a7b67023d..9b65473a2288 100644 --- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst +++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst @@ -52,9 +52,9 @@ Compressed Formats - ``V4L2_PIX_FMT_H264_MVC`` - 'M264' - H264 MVC video elementary stream. - * .. _V4L2-PIX-FMT-H264-SLICE-RAW: + * .. _V4L2-PIX-FMT-H264-SLICE: - - ``V4L2_PIX_FMT_H264_SLICE_RAW`` + - ``V4L2_PIX_FMT_H264_SLICE`` - 'S264' - H264 parsed slice data, without the start code and as extracted from the H264 bitstream. This format is adapted for diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index bb5b4926538a..39f10621c91b 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -1343,7 +1343,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) case V4L2_PIX_FMT_H264: descr = "H.264"; break; case V4L2_PIX_FMT_H264_NO_SC: descr = "H.264 (No Start Codes)"; break; case V4L2_PIX_FMT_H264_MVC: descr = "H.264 MVC"; break; - case V4L2_PIX_FMT_H264_SLICE_RAW: descr = "H.264 Parsed Slice Data"; break; + case V4L2_PIX_FMT_H264_SLICE: descr = "H.264 Parsed Slice Data"; break; case V4L2_PIX_FMT_H263: descr = "H.263"; break; case V4L2_PIX_FMT_MPEG1: descr = "MPEG-1 ES"; break; case V4L2_PIX_FMT_MPEG2: descr = "MPEG-2 ES"; break; diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index bdad87eb9d79..56ca4c9ad01c 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c @@ -46,7 +46,7 @@ void cedrus_device_run(void *priv) V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION); break; - case V4L2_PIX_FMT_H264_SLICE_RAW: + case V4L2_PIX_FMT_H264_SLICE: run.h264.decode_params = cedrus_find_control_data(ctx, V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS); run.h264.pps = cedrus_find_control_data(ctx, diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c index 681dfe3367a6..eeee3efd247b 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c @@ -38,7 +38,7 @@ static struct cedrus_format cedrus_formats[] = { .directions = CEDRUS_DECODE_SRC, }, { - .pixelformat = V4L2_PIX_FMT_H264_SLICE_RAW, + .pixelformat = V4L2_PIX_FMT_H264_SLICE, .directions = CEDRUS_DECODE_SRC, }, { @@ -104,7 +104,7 @@ static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt) switch (pix_fmt->pixelformat) { case V4L2_PIX_FMT_MPEG2_SLICE: - case V4L2_PIX_FMT_H264_SLICE_RAW: + case V4L2_PIX_FMT_H264_SLICE: /* Zero bytes per line for encoded source. */ bytesperline = 0; @@ -449,7 +449,7 @@ static int cedrus_start_streaming(struct vb2_queue *vq, unsigned int count) ctx->current_codec = CEDRUS_CODEC_MPEG2; break; - case V4L2_PIX_FMT_H264_SLICE_RAW: + case V4L2_PIX_FMT_H264_SLICE: ctx->current_codec = CEDRUS_CODEC_H264; break; diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h index e1404d78d6ff..6160a69c0143 100644 --- a/include/media/h264-ctrls.h +++ b/include/media/h264-ctrls.h @@ -14,7 +14,7 @@ #include <linux/videodev2.h> /* Our pixel format isn't stable at the moment */ -#define V4L2_PIX_FMT_H264_SLICE_RAW v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */ +#define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */ /* * This is put insanely high to avoid conflicting with controls that