| Message ID | 20190603110946.4952-6-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: uapi: h264: First batch of adjusments | expand |
Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a): > The driver only supports per-slice decoding, and in that mode > decode_params->num_slices must be set to 1 and the slice_params array > should contain only one element. What Cedrus actually needs to know is if this is first slice in frame or not. I imagine it resets some stuff internally when first slice is processed. So if driver won't get all slices of one frame at once, it can't know if this is first slice in frame or not. I guess we need additional flag for this. Best regards, Jernej > > The current code already had this limitation but it made it look like > the slice_params control was a single struct while, according to the > spec, it's actually an array. Make it more explicit by setting dims[0] > and adding a comment explaining why we have this limitation. > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Le lundi 03 juin 2019 à 23:48 +0200, Jernej Škrabec a écrit : > Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a): > > The driver only supports per-slice decoding, and in that mode > > decode_params->num_slices must be set to 1 and the slice_params array > > should contain only one element. > > What Cedrus actually needs to know is if this is first slice in frame or not. I > imagine it resets some stuff internally when first slice is processed. > > So if driver won't get all slices of one frame at once, it can't know if this > is first slice in frame or not. I guess we need additional flag for this. A first slice of a frame comes with a new timestamp, so you don't need a flag for that. regards, Nicolas > > Best regards, > Jernej > > > The current code already had this limitation but it made it look like > > the slice_params control was a single struct while, according to the > > spec, it's actually an array. Make it more explicit by setting dims[0] > > and adding a comment explaining why we have this limitation. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > >
On Mon, Jun 03, 2019 at 07:55:48PM -0400, Nicolas Dufresne wrote: > Le lundi 03 juin 2019 à 23:48 +0200, Jernej Škrabec a écrit : > > Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a): > > > The driver only supports per-slice decoding, and in that mode > > > decode_params->num_slices must be set to 1 and the slice_params array > > > should contain only one element. > > > > What Cedrus actually needs to know is if this is first slice in frame or not. I > > imagine it resets some stuff internally when first slice is processed. > > > > So if driver won't get all slices of one frame at once, it can't know if this > > is first slice in frame or not. I guess we need additional flag for this. > > A first slice of a frame comes with a new timestamp, so you don't need > a flag for that. But slices for the same frame will all have the same timestamp, so we can't use the timestamp to tell the individual slices apart. I mentioned this in that other thread, but I think it'd be useful to pass along the number of each of the slices. Drivers can use this in order to conceal errors when corrupt slices are detected during the decode operation. So if we also passed a slice index along with the offset of the slice in the bitstream, that should give us enough information to achieve both. A slice with index 0 is obviously going to be the first slice in a frame. Thierry
On Tue, 4 Jun 2019 10:12:10 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Jun 03, 2019 at 07:55:48PM -0400, Nicolas Dufresne wrote: > > Le lundi 03 juin 2019 à 23:48 +0200, Jernej Škrabec a écrit : > > > Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a): > > > > The driver only supports per-slice decoding, and in that mode > > > > decode_params->num_slices must be set to 1 and the slice_params array > > > > should contain only one element. > > > > > > What Cedrus actually needs to know is if this is first slice in frame or not. I > > > imagine it resets some stuff internally when first slice is processed. > > > > > > So if driver won't get all slices of one frame at once, it can't know if this > > > is first slice in frame or not. I guess we need additional flag for this. > > > > A first slice of a frame comes with a new timestamp, so you don't need > > a flag for that. > > But slices for the same frame will all have the same timestamp, so we > can't use the timestamp to tell the individual slices apart. I think Nicolas was suggesting to keep the last decoded slice timestamp around and check it against the new slice one. If they are different, that means we are dealing with a new frame, and this slice is the first in the frame. > > I mentioned this in that other thread, but I think it'd be useful to > pass along the number of each of the slices. Drivers can use this in > order to conceal errors when corrupt slices are detected during the > decode operation. > > So if we also passed a slice index along with the offset of the slice in > the bitstream, that should give us enough information to achieve both. A > slice with index 0 is obviously going to be the first slice in a frame. There's also the ->first_mb_in_slice field which should be 0 for the first slice and > 0 for others assuming userspace pass slices in order (might be an issue if we want to support ASO).
Le mardi 04 juin 2019 à 10:12 +0200, Thierry Reding a écrit : > On Mon, Jun 03, 2019 at 07:55:48PM -0400, Nicolas Dufresne wrote: > > Le lundi 03 juin 2019 à 23:48 +0200, Jernej Škrabec a écrit : > > > Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a): > > > > The driver only supports per-slice decoding, and in that mode > > > > decode_params->num_slices must be set to 1 and the slice_params array > > > > should contain only one element. > > > > > > What Cedrus actually needs to know is if this is first slice in frame or not. I > > > imagine it resets some stuff internally when first slice is processed. > > > > > > So if driver won't get all slices of one frame at once, it can't know if this > > > is first slice in frame or not. I guess we need additional flag for this. > > > > A first slice of a frame comes with a new timestamp, so you don't need > > a flag for that. > > But slices for the same frame will all have the same timestamp, so we > can't use the timestamp to tell the individual slices apart. > > I mentioned this in that other thread, but I think it'd be useful to > pass along the number of each of the slices. Drivers can use this in > order to conceal errors when corrupt slices are detected during the > decode operation. This is already passed as this is part of the slice header that we both pass and parse to structure. Each slice have it's first MB indicated (that standard to H264) and you can deduce the lost slice from that. > > So if we also passed a slice index along with the offset of the slice in > the bitstream, that should give us enough information to achieve both. A > slice with index 0 is obviously going to be the first slice in a frame. We do this in per-frame mode only. The offset of the slice in the bitstream is always 0 in per-slice mode, since each v4l2 input buffer is a slice. > > Thierry
Hi, Le mardi 04 juin 2019 à 10:31 -0400, Nicolas Dufresne a écrit : > Le mardi 04 juin 2019 à 10:12 +0200, Thierry Reding a écrit : > > On Mon, Jun 03, 2019 at 07:55:48PM -0400, Nicolas Dufresne wrote: > > > Le lundi 03 juin 2019 à 23:48 +0200, Jernej Škrabec a écrit : > > > > Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a): > > > > > The driver only supports per-slice decoding, and in that mode > > > > > decode_params->num_slices must be set to 1 and the slice_params array > > > > > should contain only one element. > > > > > > > > What Cedrus actually needs to know is if this is first slice in frame or not. I > > > > imagine it resets some stuff internally when first slice is processed. > > > > > > > > So if driver won't get all slices of one frame at once, it can't know if this > > > > is first slice in frame or not. I guess we need additional flag for this. > > > > > > A first slice of a frame comes with a new timestamp, so you don't need > > > a flag for that. > > > > But slices for the same frame will all have the same timestamp, so we > > can't use the timestamp to tell the individual slices apart. > > > > I mentioned this in that other thread, but I think it'd be useful to > > pass along the number of each of the slices. Drivers can use this in > > order to conceal errors when corrupt slices are detected during the > > decode operation. > > This is already passed as this is part of the slice header that we both > pass and parse to structure. Each slice have it's first MB indicated > (that standard to H264) and you can deduce the lost slice from that. > > > So if we also passed a slice index along with the offset of the slice in > > the bitstream, that should give us enough information to achieve both. A > > slice with index 0 is obviously going to be the first slice in a frame. > > We do this in per-frame mode only. The offset of the slice in the > bitstream is always 0 in per-slice mode, since each v4l2 input buffer > is a slice. I don't think we need a slice index either, we most likely already have enough information to know where we're at regarding slices position. But how about allowing an arbitrary number of slices within frame boundary in per-slice decoding mode? Cheers, Paul
On Wed, 05 Jun 2019 23:01:37 +0200 Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > Hi, > > Le mardi 04 juin 2019 à 10:31 -0400, Nicolas Dufresne a écrit : > > Le mardi 04 juin 2019 à 10:12 +0200, Thierry Reding a écrit : > > > On Mon, Jun 03, 2019 at 07:55:48PM -0400, Nicolas Dufresne wrote: > > > > Le lundi 03 juin 2019 à 23:48 +0200, Jernej Škrabec a écrit : > > > > > Dne ponedeljek, 03. junij 2019 ob 13:09:45 CEST je Boris Brezillon napisal(a): > > > > > > The driver only supports per-slice decoding, and in that mode > > > > > > decode_params->num_slices must be set to 1 and the slice_params array > > > > > > should contain only one element. > > > > > > > > > > What Cedrus actually needs to know is if this is first slice in frame or not. I > > > > > imagine it resets some stuff internally when first slice is processed. > > > > > > > > > > So if driver won't get all slices of one frame at once, it can't know if this > > > > > is first slice in frame or not. I guess we need additional flag for this. > > > > > > > > A first slice of a frame comes with a new timestamp, so you don't need > > > > a flag for that. > > > > > > But slices for the same frame will all have the same timestamp, so we > > > can't use the timestamp to tell the individual slices apart. > > > > > > I mentioned this in that other thread, but I think it'd be useful to > > > pass along the number of each of the slices. Drivers can use this in > > > order to conceal errors when corrupt slices are detected during the > > > decode operation. > > > > This is already passed as this is part of the slice header that we both > > pass and parse to structure. Each slice have it's first MB indicated > > (that standard to H264) and you can deduce the lost slice from that. > > > > > So if we also passed a slice index along with the offset of the slice in > > > the bitstream, that should give us enough information to achieve both. A > > > slice with index 0 is obviously going to be the first slice in a frame. > > > > We do this in per-frame mode only. The offset of the slice in the > > bitstream is always 0 in per-slice mode, since each v4l2 input buffer > > is a slice. > > I don't think we need a slice index either, we most likely already have > enough information to know where we're at regarding slices position. > > But how about allowing an arbitrary number of slices within frame > boundary in per-slice decoding mode? Yep, will send a v2 taking that case into consideration.
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c index 378032fe71f9..3661c6a04864 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c @@ -49,6 +49,12 @@ static const struct cedrus_control cedrus_controls[] = { { .cfg.id = V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS, .cfg.elem_size = sizeof(struct v4l2_ctrl_h264_slice_params), + /* + * This driver does not support per-frame decoding (yet?). + * Allow only on per-slice decoding operation, which implies + * that only 1 slice param is passed per decoding operation. + */ + .cfg.dims[0] = 1, .codec = CEDRUS_CODEC_H264, .required = true, },
The driver only supports per-slice decoding, and in that mode decode_params->num_slices must be set to 1 and the slice_params array should contain only one element. The current code already had this limitation but it made it look like the slice_params control was a single struct while, according to the spec, it's actually an array. Make it more explicit by setting dims[0] and adding a comment explaining why we have this limitation. Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> --- drivers/staging/media/sunxi/cedrus/cedrus.c | 6 ++++++ 1 file changed, 6 insertions(+)