From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 6978547EF0 for ; Thu, 29 Feb 2024 12:13:23 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id BB28368CE27; Thu, 29 Feb 2024 14:13:20 +0200 (EET) Received: from haasn.dev (haasn.dev [78.46.187.166]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 6E39568C775 for ; Thu, 29 Feb 2024 14:13:14 +0200 (EET) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=haasn.xyz; s=mail; t=1709208794; bh=Fu8YO/m6lles5mcYomLMNNFdcDaGE4gwrf/nbpPrWjc=; h=Date:From:To:Subject:In-Reply-To:References:From; b=YdT7UdS5TqDgo+NNFDJV5rTIaPAAEzf2JF/wBXXsluEwyr4oaL5uW2EgiEGJJACui r9neuBeBiv/WDlNRvbJrfzSX/vPUR1MynFkCy9OdMidjGNgFtPuY9l0uRIm4h5GV2I uAu5k17YCju5Rvjs1eq2A7NtIskznVGv7MIW3zLU= Received: from haasn.dev (unknown [10.30.0.2]) by haasn.dev (Postfix) with ESMTP id 2582440292 for ; Thu, 29 Feb 2024 13:13:14 +0100 (CET) Date: Thu, 29 Feb 2024 13:13:13 +0100 Message-ID: <20240229131313.GB40276@haasn.xyz> From: Niklas Haas To: ffmpeg-devel@ffmpeg.org In-Reply-To: References: <20240226143521.57913-1-ffmpeg@haasn.xyz> <20240228165605.GB85477@haasn.xyz> <20240229120301.GB20801@haasn.xyz> MIME-Version: 1.0 Content-Disposition: inline Subject: Re: [FFmpeg-devel] [PATCH 1/4] avcodec/aom_film_grain: add AOM film grain synthesis X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Thu, 29 Feb 2024 12:36:36 +0100 Andreas Rheinhardt wrote: > Niklas Haas: > > +#if BIT_DEPTH > 8 > > +# define entry int16_t > > +# define bitdepth_max ((1 << bitdepth) - 1) > > +# define HBD_DECL , const int bitdepth > > +# define HBD_CALL , bitdepth > > +# define SCALING_SIZE 4096 > > +#else > > +# define entry int8_t > > +# define bitdepth 8 > > +# define bitdepth_max UINT8_MAX > > Why are you using signed types, but unsigned max values? `entry` is the type of a (signed) film grain LUT entry, which is distinct from the maximum value for the image bit-depth. Consider the canonical usage as: entry grain_lut[]; pixel = av_clip( src_pixel + grain_lut[idx], 0, bitdepth_max ); > > +# define HBD_DECL > > +# define HBD_CALL > > +# define SCALING_SIZE 256 > > +#endif > > + > > +// Symbols that do not depend on bit depth > > +#ifndef PXSTRIDE > > +# define PXSTRIDE(x) ((x) / sizeof(pixel)) > > There are several things that are wrong with this pattern: > 1. sizeof is size_t and this has a conversion rank >= int/ptrdiff_t on > all systems I am aware of, therefore the division will be performed with > unsigned types, i.e. it is a logical right shift. And this will just not > work with negative linesizes (but given that pointer arithmetic via a > pixel* commonly involves a multiplication by sizeof(pixel), it often > happens to work in practice, but e.g. UBSan warns about this). > 2. It presumes that linesize is always a multiple of the sizeof of the > underlying type. This need not be so. Imagine a system where there are > no alignment requirements whatsoever (i.e. the required alignment of > every type is 1 and vector instructions (if existing) also have no > alignment requirement). Then it is legal for our callers to use frames > where linesize can be anything. > 3. Even if linesize is always a multiple of sizeof(pixel), the compiler > does not know this and therefore will have to mask the low bits. So > instead of > src_row + (y) * PXSTRIDE(stride) + (x) + bx > use > (const pixel*)((const char*)src_row + (y) * stride) + (x) + bx > > (Please don't cast const away on intermediate pointers when changing this.) Thanks. The code was copied as-is from dav1d (which also supports negative strides, but I guess they don't care about this particular UB?) I will change it to the pattern you recommended. > > + // Copy over the non-modified planes > > + if (!data->num_y_points) { > > + av_image_copy_plane(out_frame->data[0], out_frame->linesize[0], > > + in_frame->data[0], in_frame->linesize[0], > > + out_frame->width * sizeof(pixel), out_frame->height); > > + } > > + for (int uv = 0; uv < 2; uv++) { > > + if (!data->num_uv_points[uv]) { > > + av_image_copy_plane(out_frame->data[1+uv], out_frame->linesize[1+uv], > > + in_frame->data[1+uv], in_frame->linesize[1+uv], > > + AV_CEIL_RSHIFT(out_frame->width, subx) * sizeof(pixel), > > + AV_CEIL_RSHIFT(out_frame->height, suby)); > > + } > > + } > > + > > This is generic and can be moved out of the template. Done. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".