From: Maxime Ripard <maxime.ripard@bootlin.com> To: Jernej Skrabec <jernej.skrabec@siol.net> Cc: paul.kocialkowski@bootlin.com, wens@csie.org, mchehab@kernel.org, gregkh@linuxfoundation.org, linux-media@vger.kernel.org, devel@driverdev.osuosl.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/7] media: cedrus: Fix decoding for some H264 videos Date: Mon, 3 Jun 2019 13:55:36 +0200 [thread overview] Message-ID: <20190603115536.j5lan6wtmbxpoe2k@flea> (raw) In-Reply-To: <20190530211516.1891-4-jernej.skrabec@siol.net> [-- Attachment #1: Type: text/plain, Size: 2310 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Maxime Ripard <maxime.ripard@bootlin.com> To: Jernej Skrabec <jernej.skrabec@siol.net> Cc: devel@driverdev.osuosl.org, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, paul.kocialkowski@bootlin.com, wens@csie.org, mchehab@kernel.org, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Subject: Re: [PATCH 3/7] media: cedrus: Fix decoding for some H264 videos Date: Mon, 3 Jun 2019 13:55:36 +0200 [thread overview] Message-ID: <20190603115536.j5lan6wtmbxpoe2k@flea> (raw) In-Reply-To: <20190530211516.1891-4-jernej.skrabec@siol.net> [-- Attachment #1.1: Type: text/plain, Size: 2310 bytes --] 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 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-06-03 11:55 UTC|newest] Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-30 21:15 [PATCH 0/7] media: cedrus: Improvements/cleanup Jernej Skrabec 2019-05-30 21:15 ` Jernej Skrabec 2019-05-30 21:15 ` [PATCH 1/7] media: cedrus: Disable engine after each slice decoding Jernej Skrabec 2019-05-30 21:15 ` Jernej Skrabec 2019-06-03 11:38 ` Maxime Ripard 2019-06-03 11:38 ` Maxime Ripard 2019-08-12 13:28 ` Ezequiel Garcia 2019-08-12 13:28 ` Ezequiel Garcia 2019-08-12 13:28 ` Ezequiel Garcia 2019-05-30 21:15 ` [PATCH 2/7] media: cedrus: Fix H264 default reference index count Jernej Skrabec 2019-05-30 21:15 ` Jernej Skrabec 2019-06-03 11:46 ` Maxime Ripard 2019-06-03 11:46 ` Maxime Ripard 2019-06-03 15:34 ` Jernej Škrabec 2019-06-03 15:34 ` Jernej Škrabec 2019-05-30 21:15 ` [PATCH 3/7] media: cedrus: Fix decoding for some H264 videos Jernej Skrabec 2019-05-30 21:15 ` Jernej Skrabec 2019-06-03 11:55 ` Maxime Ripard [this message] 2019-06-03 11:55 ` Maxime Ripard 2019-06-03 15:37 ` Jernej Škrabec 2019-06-03 15:37 ` Jernej Škrabec 2019-06-06 12:45 ` Dan Carpenter 2019-06-06 12:45 ` Dan Carpenter 2019-05-30 21:15 ` [PATCH 4/7] media: cedrus: Remove dst_bufs from context Jernej Skrabec 2019-05-30 21:15 ` Jernej Skrabec 2019-06-03 12:13 ` Maxime Ripard 2019-06-03 12:13 ` Maxime Ripard 2019-06-05 21:07 ` Paul Kocialkowski 2019-06-05 21:07 ` Paul Kocialkowski 2019-08-12 13:42 ` Ezequiel Garcia 2019-08-12 13:42 ` Ezequiel Garcia 2019-08-12 13:42 ` Ezequiel Garcia 2019-05-30 21:15 ` [PATCH 5/7] media: cedrus: Don't set chroma size for scale & rotation Jernej Skrabec 2019-05-30 21:15 ` Jernej Skrabec 2019-06-05 21:08 ` Paul Kocialkowski 2019-06-05 21:08 ` Paul Kocialkowski 2019-05-30 21:15 ` [PATCH 6/7] media: cedrus: Add infra for extra buffers connected to capture buffers Jernej Skrabec 2019-05-30 21:15 ` Jernej Skrabec 2019-06-03 12:18 ` Maxime Ripard 2019-06-03 12:18 ` Maxime Ripard 2019-06-03 15:48 ` Jernej Škrabec 2019-06-03 15:48 ` Jernej Škrabec 2019-06-05 21:10 ` Paul Kocialkowski 2019-06-05 21:10 ` Paul Kocialkowski 2019-06-05 21:52 ` Jernej Škrabec 2019-06-05 21:52 ` Jernej Škrabec 2019-06-06 8:33 ` Maxime Ripard 2019-06-06 8:33 ` Maxime Ripard 2019-05-30 21:15 ` [PATCH 7/7] media: cedrus: Improve H264 memory efficiency Jernej Skrabec 2019-05-30 21:15 ` Jernej Skrabec 2019-06-03 12:23 ` Maxime Ripard 2019-06-03 12:23 ` Maxime Ripard 2019-06-03 16:37 ` Jernej Škrabec 2019-06-03 16:37 ` Jernej Škrabec 2019-06-05 21:12 ` Paul Kocialkowski 2019-06-05 21:12 ` Paul Kocialkowski 2019-08-12 12:12 ` [PATCH 0/7] media: cedrus: Improvements/cleanup Hans Verkuil 2019-08-12 12:12 ` Hans Verkuil 2019-08-12 12:12 ` Hans Verkuil 2019-08-12 12:21 ` Jernej Škrabec 2019-08-12 12:21 ` Jernej Škrabec 2019-08-12 12:21 ` Jernej Škrabec 2019-08-12 13:55 ` Maxime Ripard 2019-08-12 13:55 ` Maxime Ripard 2019-08-12 13:55 ` Maxime Ripard
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190603115536.j5lan6wtmbxpoe2k@flea \ --to=maxime.ripard@bootlin.com \ --cc=devel@driverdev.osuosl.org \ --cc=gregkh@linuxfoundation.org \ --cc=jernej.skrabec@siol.net \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=mchehab@kernel.org \ --cc=paul.kocialkowski@bootlin.com \ --cc=wens@csie.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.