Dne sreda, 06. marec 2019 ob 11:57:08 CET je Maxime Ripard napisal(a):

Hi,

On Tue, Mar 05, 2019 at 06:05:08PM +0100, Jernej Škrabec wrote:

quoted

Dne torek, 05. marec 2019 ob 11:17:32 CET je Maxime Ripard napisal(a):

quoted

Hi Jernej,

On Wed, Feb 20, 2019 at 06:50:54PM +0100, Jernej Škrabec wrote:

quoted

I really wanted to do another review on previous series but got
distracted
by analyzing one particulary troublesome H264 sample. It still doesn't
work correctly, so I would ask you if you can test it with your stack
(it
might be userspace issue):

http://jernej.libreelec.tv/videos/problematic/test.mkv

Please take a look at my comments below.

I'd really prefer to focus on getting this merged at this point, and
then fixing odd videos and / or setups we can find later
on. Especially when new stacks are going to be developped on top of
this, I'm sure we're going to have plenty of bugs to address :)

I forgot to mention, you can add:
Reviewed-by: Jernej Skrabec <redacted>

once you fix issues.

Great, thanks :)

quoted

quoted

quoted

quoted

+	for (i = 0; i < ARRAY_SIZE(pred_weight->weight_factors); i++) {
+		const struct v4l2_h264_weight_factors *factors =
+			&pred_weight->weight_factors[i];
+
+		for (j = 0; j < ARRAY_SIZE(factors->luma_weight); j++)

{

quoted

quoted

quoted

+			u32 val;
+
+			val = ((factors->luma_offset[j] & 0x1ff) <<

16)

quoted

quoted

quoted

+				(factors->luma_weight[j] & 

0x1ff);

quoted

quoted

quoted

quoted

+			cedrus_write(dev, VE_AVC_SRAM_PORT_DATA,

val);

You should cast offset varible to wider type. Currently some videos
which
use prediction weight table don't work for me, unless offset is casted
to
u32 first. Shifting 8 bit variable for 16 places gives you 0 every
time.

I'll do it.

quoted

Luma offset and weight are defined as s8, so having wider mask doesn't
really make sense. However, I think weight should be s16 anyway,
because
standard says that it's value could be 2^denominator for default value
or
in range -128..127. Worst case would be 2^7 = 128 and -128. To cover
both
values you need at least 9 bits.

But if I understood the spec right, in that case you would just have
the denominator set, and not the offset, while the offset is used if
you don't use the default formula (and therefore remains in the -128
127 range which is covered by the s8), right?

Yeah, default offset is 0 and s8 is sufficient for that. I'm talking about
weight. Default weight is "1 << denominator", which might be 1 << 7 or
128.

We could also add a flag, which would signal default table. In that case
we
could just set a bit to tell VPU to use default values. Even if some VPUs
need default table to be set explicitly, it's very easy to calculate
values as mentioned in previous paragraph.

Yeah, sorry, I meant weight. Would that make any difference? Can we
have situations where both the denominator and the weight would be
set, with the weight set to 128?

Yes, that's the case when default weight is used and (log2) denominator is 7. 
Weight is then "1 << denominator".

I've checked in the libva and ffmpeg, and libva uses a short, while
ffmpeg uses an int, both for the weight and offset. For consistency I
guess we could change both to shorts just like libva?

What do you think?

Yes, that would work for me. Maybe someone else has opinion about this? But I 
think there is a reason why libva uses shorts.

Best regards,
Jernej



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel