Re: [PATCH v7 02/11] media: uapi: h264: Rename pixel format

From: Hans Verkuil
Date: Thu Aug 22 2019 - 09:48:09 EST

Next message: Andrew Lunn: "Re: [PATCH] i2c: ocores: use request_any_context_irq() to register IRQ handler"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v6 3/4] dt-bindings: arm: fsl: Add Kontron i.MX6UL N6310 compatibles"
In reply to: Paul Kocialkowski: "Re: [PATCH v7 02/11] media: uapi: h264: Rename pixel format"
Next in thread: Ezequiel Garcia: "Re: [PATCH v7 02/11] media: uapi: h264: Rename pixel format"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxx>
>>>> Tested-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>>>> ---
>>>> 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
>>>>
>>>
>>
>

Next message: Andrew Lunn: "Re: [PATCH] i2c: ocores: use request_any_context_irq() to register IRQ handler"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v6 3/4] dt-bindings: arm: fsl: Add Kontron i.MX6UL N6310 compatibles"
In reply to: Paul Kocialkowski: "Re: [PATCH v7 02/11] media: uapi: h264: Rename pixel format"
Next in thread: Ezequiel Garcia: "Re: [PATCH v7 02/11] media: uapi: h264: Rename pixel format"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]