| Message ID | 20190530211516.1891-4-jernej.skrabec@siol.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: cedrus: Improvements/cleanup | expand |
Hi, On Thu, May 30, 2019 at 11:15:12PM +0200, Jernej Skrabec wrote: > It seems that for some H264 videos at least one bitstream parsing > trigger must be called in order to be decoded correctly. There is no > explanation why this helps, but it was observed that two sample videos > with this fix are now decoded correctly and there is no regression with > others. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > I have two samples which are fixed by this: > http://jernej.libreelec.tv/videos/h264/test.mkv > http://jernej.libreelec.tv/videos/h264/Dredd%20%E2%80%93%20DTS%20Sound%20Check%20DTS-HD%20MA%207.1.m2ts > > Although second one also needs support for multi-slice frames, which is not yet implemented here. > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 22 ++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > index cc8d17f211a1..d0ee3f90ff46 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > @@ -6,6 +6,7 @@ > * Copyright (c) 2018 Bootlin > */ > > +#include <linux/delay.h> > #include <linux/types.h> > > #include <media/videobuf2-dma-contig.h> > @@ -289,6 +290,20 @@ static void cedrus_write_pred_weight_table(struct cedrus_ctx *ctx, > } > } We should have a comment here explaining why that is needed > +static void cedrus_skip_bits(struct cedrus_dev *dev, int num) > +{ > + for (; num > 32; num -= 32) { > + cedrus_write(dev, VE_H264_TRIGGER_TYPE, 0x3 | (32 << 8)); Using defines here would be great > + while (cedrus_read(dev, VE_H264_STATUS) & (1 << 8)) > + udelay(1); > + } A new line here would be great > + if (num > 0) { > + cedrus_write(dev, VE_H264_TRIGGER_TYPE, 0x3 | (num << 8)); > + while (cedrus_read(dev, VE_H264_STATUS) & (1 << 8)) > + udelay(1); > + } Can't we make that a bit simpler by not duplicating the loop? Something like: int current = 0; while (current < num) { int tmp = min(num - current, 32); cedrus_write(dev, VE_H264_TRIGGER_TYPE, 0x3 | (current << 8)) while (...) ... current += tmp; } Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Dne ponedeljek, 03. junij 2019 ob 13:55:36 CEST je Maxime Ripard napisal(a): > Hi, > > On Thu, May 30, 2019 at 11:15:12PM +0200, Jernej Skrabec wrote: > > It seems that for some H264 videos at least one bitstream parsing > > trigger must be called in order to be decoded correctly. There is no > > explanation why this helps, but it was observed that two sample videos > > with this fix are now decoded correctly and there is no regression with > > others. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > I have two samples which are fixed by this: > > http://jernej.libreelec.tv/videos/h264/test.mkv > > http://jernej.libreelec.tv/videos/h264/Dredd%20%E2%80%93%20DTS%20Sound%20C > > heck%20DTS-HD%20MA%207.1.m2ts > > > > Although second one also needs support for multi-slice frames, which is > > not yet implemented here.> > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 22 ++++++++++++++++--- > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index > > cc8d17f211a1..d0ee3f90ff46 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > @@ -6,6 +6,7 @@ > > > > * Copyright (c) 2018 Bootlin > > */ > > > > +#include <linux/delay.h> > > > > #include <linux/types.h> > > > > #include <media/videobuf2-dma-contig.h> > > > > @@ -289,6 +290,20 @@ static void cedrus_write_pred_weight_table(struct > > cedrus_ctx *ctx,> > > } > > > > } > > We should have a comment here explaining why that is needed ok. > > > +static void cedrus_skip_bits(struct cedrus_dev *dev, int num) > > +{ > > + for (; num > 32; num -= 32) { > > + cedrus_write(dev, VE_H264_TRIGGER_TYPE, 0x3 | (32 << 8)); > > Using defines here would be great Yes, sorry about that. > > > + while (cedrus_read(dev, VE_H264_STATUS) & (1 << 8)) > > + udelay(1); > > + } > > A new line here would be great > > > + if (num > 0) { > > + cedrus_write(dev, VE_H264_TRIGGER_TYPE, 0x3 | (num << 8)); > > + while (cedrus_read(dev, VE_H264_STATUS) & (1 << 8)) > > + udelay(1); > > + } > > Can't we make that a bit simpler by not duplicating the loop? > > Something like: > > int current = 0; > > while (current < num) { > int tmp = min(num - current, 32); > > cedrus_write(dev, VE_H264_TRIGGER_TYPE, 0x3 | (current << 8)) > while (...) > ... > > current += tmp; > } Yes, that looks better, I'll change it in next version. Best regards, Jernej
On Mon, Jun 03, 2019 at 05:37:17PM +0200, Jernej Škrabec wrote: > Dne ponedeljek, 03. junij 2019 ob 13:55:36 CEST je Maxime Ripard napisal(a): > > int current = 0; > > > > while (current < num) { > > int tmp = min(num - current, 32); > > > > cedrus_write(dev, VE_H264_TRIGGER_TYPE, 0x3 | (current << 8)) ^^^^^^^ This should be "tmp << 8" instead of "current << 8" though. > > while (...) > > ... > > > > current += tmp; > > } > regards, dan carpenter
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index cc8d17f211a1..d0ee3f90ff46 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c @@ -6,6 +6,7 @@ * Copyright (c) 2018 Bootlin */ +#include <linux/delay.h> #include <linux/types.h> #include <media/videobuf2-dma-contig.h> @@ -289,6 +290,20 @@ static void cedrus_write_pred_weight_table(struct cedrus_ctx *ctx, } } +static void cedrus_skip_bits(struct cedrus_dev *dev, int num) +{ + for (; num > 32; num -= 32) { + cedrus_write(dev, VE_H264_TRIGGER_TYPE, 0x3 | (32 << 8)); + while (cedrus_read(dev, VE_H264_STATUS) & (1 << 8)) + udelay(1); + } + if (num > 0) { + cedrus_write(dev, VE_H264_TRIGGER_TYPE, 0x3 | (num << 8)); + while (cedrus_read(dev, VE_H264_STATUS) & (1 << 8)) + udelay(1); + } +} + static void cedrus_set_params(struct cedrus_ctx *ctx, struct cedrus_run *run) { @@ -299,12 +314,11 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, struct vb2_buffer *src_buf = &run->src->vb2_buf; struct cedrus_dev *dev = ctx->dev; dma_addr_t src_buf_addr; - u32 offset = slice->header_bit_size; - u32 len = (slice->size * 8) - offset; + u32 len = slice->size * 8; u32 reg; cedrus_write(dev, VE_H264_VLD_LEN, len); - cedrus_write(dev, VE_H264_VLD_OFFSET, offset); + cedrus_write(dev, VE_H264_VLD_OFFSET, 0); src_buf_addr = vb2_dma_contig_plane_dma_addr(src_buf, 0); cedrus_write(dev, VE_H264_VLD_END, @@ -323,6 +337,8 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, cedrus_write(dev, VE_H264_TRIGGER_TYPE, VE_H264_TRIGGER_TYPE_INIT_SWDEC); + cedrus_skip_bits(dev, slice->header_bit_size); + if (((pps->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && (slice->slice_type == V4L2_H264_SLICE_TYPE_P || slice->slice_type == V4L2_H264_SLICE_TYPE_SP)) ||
It seems that for some H264 videos at least one bitstream parsing trigger must be called in order to be decoded correctly. There is no explanation why this helps, but it was observed that two sample videos with this fix are now decoded correctly and there is no regression with others. Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- I have two samples which are fixed by this: http://jernej.libreelec.tv/videos/h264/test.mkv http://jernej.libreelec.tv/videos/h264/Dredd%20%E2%80%93%20DTS%20Sound%20Check%20DTS-HD%20MA%207.1.m2ts Although second one also needs support for multi-slice frames, which is not yet implemented here. .../staging/media/sunxi/cedrus/cedrus_h264.c | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)