Re: [PATCH v4 2/2] media: cedrus: Add H264 decoding support

[Maxime Ripard]

Hi,

On Tue, Mar 05, 2019 at 06:05:08PM +0100, Jernej Åkrabec wrote:
> Dne torek, 05. marec 2019 ob 11:17:32 CET je Maxime Ripard napisal(a):
> > Hi Jernej,
> > 
> > On Wed, Feb 20, 2019 at 06:50:54PM +0100, Jernej Åkrabec wrote:
> > > 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 <jernej.skrabec@xxxxxxxx>
> 
> once you fix issues.

Great, thanks :)

> > > > +	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++) 
> {
> > > > +			u32 val;
> > > > +
> > > > +			val = ((factors->luma_offset[j] & 0x1ff) << 
> 16)
> > > > 
> > > > +				(factors->luma_weight[j] & 0x1ff);
> > > > +			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.
> > 
> > > 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?

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?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature