From patchwork Wed Mar 14 14:59:59 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: James Almer X-Patchwork-Id: 7994 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.2.1.70 with SMTP id c67csp150992jad; Wed, 14 Mar 2018 08:00:14 -0700 (PDT) X-Google-Smtp-Source: AG47ELun5sjt4LwGqKLgGAQRh52l0qbgcIoUpAJawBG2zYEMEJ7z3MtoziJ6zeX9u+tSbpvDNvPS X-Received: by 10.223.177.132 with SMTP id q4mr4068554wra.27.1521039614503; Wed, 14 Mar 2018 08:00:14 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1521039614; cv=none; d=google.com; s=arc-20160816; b=aNdoMbzCjfQ9ErfzFy9udZC5jzHP4KSUEWM8bIvxL1pz7bgtB9FXzVfwqianfhRMui 83JV4LvJeapL7hX9gOgsP3CxrTOBg2Vp++B0hG/1JcJAaJTAuz92J8ougn3b9VY6oQjR e6i41qkEsLirBzJKcGjgNzaTsmRBl6vMbkL0dDMPVr9VeEpeyliqe1Rl4G+fnt7AoRoL sMfyyxOQczJf2bNgMyFNxwn7noCUgqChQ0KLvF7VNOOf7PRQrDf0Ecb0fMRHpo5TR9KU Jgt7/ieTKzT9cxP1lUAMA1gNReijkL0den0RkvbqnRL3pls9BEnOD3rOW3UHmsMrBoTS 4RSQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:content-transfer-encoding:reply-to:list-subscribe :list-help:list-post:list-archive:list-unsubscribe:list-id :precedence:subject:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:to:dkim-signature :delivered-to:arc-authentication-results; bh=TsMuI1nRJqlxNfFgdNKbFnwbQhR0EFKSAwe2JcMREVs=; b=yrchStbDrgYIABknoEQTtO1yUbkemjXCXBY5/0Kp+8VzAVRac6EMaJ6Zl7HA+peDDg 3ZpJYuZZ79t5hFbu+cxfdj8fDPfLIqTk4vBQcFft4Vko+ATBMd5G78O2JlecrmH/KCxN GSVrNyT9zVEp6vrf2AshTl2uqoBo6dhVTdIJUzOO40SbldCktCRKhOBcHhZ30JALiZJt GVqeGY14WocleDxQGcw4f5NGVC+WipwFt/FOlkAlDt5cz/1r0mLr42LY2pq50Q5l366F gs6lrr1EEQSGvTGPMbni7YZ++43ehyzXhhZeijsre8N7+Kl/hk7pe/36AI1kWNsEGxdD aIEg== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@gmail.com header.s=20161025 header.b=WZz4ocSj; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org; dmarc=fail (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id h63si1034295wmh.118.2018.03.14.08.00.13; Wed, 14 Mar 2018 08:00:14 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@gmail.com header.s=20161025 header.b=WZz4ocSj; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org; dmarc=fail (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 4EE8C68A1EE; Wed, 14 Mar 2018 17:00:00 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-qt0-f172.google.com (mail-qt0-f172.google.com [209.85.216.172]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 85062689F6E for ; Wed, 14 Mar 2018 16:59:53 +0200 (EET) Received: by mail-qt0-f172.google.com with SMTP id s48so3713509qtb.10 for ; Wed, 14 Mar 2018 08:00:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=n87NhkrrmC9INp+Lc3yew9RbFRszIrCJL1tK3+oDO0E=; b=WZz4ocSjOk3bhzFTvvmNfIOxIV0Jg4FmsAqjd3g++soClNax+m+s62MCK6m5WZYKPJ +bt7g5NNPZRcC+ztb1omSfltvaXmWAYuIl+Vbl8qOAvR1kTVrTH9IWvMTMsFU8FbZN3j xc1p/7SbE5rSoHaPTiW2pVBQxcUANra9uW7PT+VdidL4rMI1HQMIzs/eidI+AdwGKTGh 0rRE4CN6VBSCadc6h+hGOfa/jF3zPT3MgMuge+RYgWzJlNotbg/LPLv/Fc1rE1cgPXNq Ul7/UNFing0TcxCiWZ/MovGuMRS/+4xHjQGALNvCcbyQMCuI9YlOOV/Pi60oTteXat2v lVzg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=n87NhkrrmC9INp+Lc3yew9RbFRszIrCJL1tK3+oDO0E=; b=mu0cy2u59BxsK6tftfHDZfMETS9fXEjl3bSgLVlL3wofce4Et08+9FITlu/i6kJlXL psp6OPhG39dXlcslJMZCrxCprTztBdeMg07N04lsn7YiMFRqTEuQpriWApAY0ivi2yZO Ro5zitToVODevb6WPCfkIa6rCGYj9e2Hu2qJZuoYT4eHL1XetVQKGJZfb3/p6gSoFcVg RnW/23gF4V6fWWGg8dw1QELrDjut1zFEkZEoD5+uPOdW16krVJj+t5YMqxGL/OFWIsxM L5F5WjTpbO5KSqNa769p2D6+AjyysCPK+q93Xf1xS53dlMi7h8NTnRIrTR0gAAYCfuln uhwQ== X-Gm-Message-State: AElRT7E8vdJgvN7YYSD1XIeisohEizG+sB+IJ5E2W40c7gaKHcqBKCkm NWVj5jhxDheKpI5QuC96AMJyjOvq X-Received: by 10.200.41.18 with SMTP id y18mr7691810qty.181.1521039603635; Wed, 14 Mar 2018 08:00:03 -0700 (PDT) Received: from [192.168.0.4] ([190.188.171.140]) by smtp.gmail.com with ESMTPSA id 1sm2097002qtr.85.2018.03.14.08.00.02 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 14 Mar 2018 08:00:03 -0700 (PDT) To: ffmpeg-devel@ffmpeg.org References: <20180313234856.11108-1-jamrial@gmail.com> <20180314081448.54cc5ee4@debian> <20180314153533.58bcc2e8@debian> From: James Almer Message-ID: <57b66b97-16be-64cf-bcc7-631e84e83797@gmail.com> Date: Wed, 14 Mar 2018 11:59:59 -0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180314153533.58bcc2e8@debian> Content-Language: en-US Subject: Re: [FFmpeg-devel] [PATCH] avutil/buffer: add av_buffer_fast_alloc() X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 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 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" On 3/14/2018 11:35 AM, wm4 wrote: > On Wed, 14 Mar 2018 11:13:52 -0300 > James Almer wrote: > >> On 3/14/2018 4:14 AM, wm4 wrote: >>> On Tue, 13 Mar 2018 20:48:56 -0300 >>> James Almer wrote: >>> >>>> Same concept as av_fast_malloc(). If the buffer passed to it is writable >>>> and big enough it will be reused, otherwise a new one will be allocated >>>> instead. >>>> >>>> Signed-off-by: James Almer >>>> --- >>>> TODO: Changelog and APIChanges entries, version bump. >>>> >>>> libavutil/buffer.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> libavutil/buffer.h | 42 ++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 105 insertions(+) >>>> >>>> diff --git a/libavutil/buffer.c b/libavutil/buffer.c >>>> index 8d1aa5fa84..16ce1b82e2 100644 >>>> --- a/libavutil/buffer.c >>>> +++ b/libavutil/buffer.c >>>> @@ -215,6 +215,69 @@ int av_buffer_realloc(AVBufferRef **pbuf, int size) >>>> return 0; >>>> } >>>> >>>> +static av_always_inline int buffer_fast_alloc(AVBufferRef **pbuf, int size, int zero_alloc) >>>> +{ >>>> + AVBufferRef *buf = *pbuf; >>>> + AVBuffer *b; >>>> + uint8_t *data; >>>> + >>>> + if (!buf || !av_buffer_is_writable(buf) || buf->data != buf->buffer->data) { >>>> + /* Buffer can't be reused, and neither can the entire AVBufferRef. >>>> + * Unref the latter and alloc a new one. */ >>>> + av_buffer_unref(pbuf); >>>> + >>>> + buf = av_buffer_alloc(size); >>>> + if (!buf) >>>> + return AVERROR(ENOMEM); >>>> + >>>> + if (zero_alloc) >>>> + memset(buf->data, 0, size); >>>> + >>>> + *pbuf = buf; >>>> + return 0; >>>> + } >>>> + b = buf->buffer; >>>> + >>>> + if (size <= b->size) { >>>> + /* Buffer can be reused. Update the size of AVBufferRef but leave the >>>> + * AVBuffer untouched. */ >>>> + buf->size = FFMAX(0, size); >>>> + return 0; >>>> + } >>>> + >>>> + /* Buffer can't be reused, but there's no need to alloc new AVBuffer and >>>> + * AVBufferRef structs. Free the existing buffer, allocate a new one, and >>>> + * reset AVBuffer and AVBufferRef to default values. */ >>>> + b->free(b->opaque, b->data); >>>> + b->free = av_buffer_default_free; >>>> + b->opaque = NULL; >>>> + b->flags = 0; >>>> + >>>> + data = av_malloc(size); >>>> + if (!data) { >>>> + av_buffer_unref(pbuf); >>>> + return AVERROR(ENOMEM); >>>> + } >>>> + >>>> + if (zero_alloc) >>>> + memset(data, 0, size); >>>> + >>>> + b->data = buf->data = data; >>>> + b->size = buf->size = size; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +int av_buffer_fast_alloc(AVBufferRef **pbuf, int size) >>>> +{ >>>> + return buffer_fast_alloc(pbuf, size, 0); >>>> +} >>>> + >>>> +int av_buffer_fast_allocz(AVBufferRef **pbuf, int size) >>>> +{ >>>> + return buffer_fast_alloc(pbuf, size, 1); >>>> +} >>>> + >>>> AVBufferPool *av_buffer_pool_init2(int size, void *opaque, >>>> AVBufferRef* (*alloc)(void *opaque, int size), >>>> void (*pool_free)(void *opaque)) >>>> diff --git a/libavutil/buffer.h b/libavutil/buffer.h >>>> index 73b6bd0b14..1166017d22 100644 >>>> --- a/libavutil/buffer.h >>>> +++ b/libavutil/buffer.h >>>> @@ -197,6 +197,48 @@ int av_buffer_make_writable(AVBufferRef **buf); >>>> */ >>>> int av_buffer_realloc(AVBufferRef **buf, int size); >>>> >>>> +/** >>>> + * Allocate a buffer, reusing the given one if writable and large enough. >>>> + * >>>> + * @code{.c} >>>> + * AVBufferRef *buf = ...; >>>> + * int ret = av_buffer_fast_alloc(&buf, size); >>>> + * if (ret < 0) { >>>> + * // Allocation failed; buf already freed >>>> + * return ret; >>>> + * } >>>> + * @endcode >>>> + * >>>> + * @param buf A buffer reference. *buf may be NULL. On success, a new buffer >>>> + * reference will be written in its place. On failure, it will be >>>> + * unreferenced and set to NULL. >>>> + * @param size Required buffer size. >>>> + * >>>> + * @return 0 on success, a negative AVERROR on failure. >>>> + * >>>> + * @see av_buffer_realloc() >>>> + * @see av_buffer_fast_allocz() >>>> + */ >>>> +int av_buffer_fast_alloc(AVBufferRef **buf, int size); >>>> + >>>> +/** >>>> + * Allocate and clear a buffer, reusing the given one if writable and large >>>> + * enough. >>>> + * >>>> + * Like av_buffer_fast_alloc(), but all newly allocated space is initially >>>> + * cleared. Reused buffer is not cleared. >>>> + * >>>> + * @param buf A buffer reference. *buf may be NULL. On success, a new buffer >>>> + * reference will be written in its place. On failure, it will be >>>> + * unreferenced and set to NULL. >>>> + * @param size Required buffer size. >>>> + * >>>> + * @return 0 on success, a negative AVERROR on failure. >>>> + * >>>> + * @see av_buffer_fast_alloc() >>>> + */ >>>> +int av_buffer_fast_allocz(AVBufferRef **buf, int size); >>>> + >>>> /** >>>> * @} >>>> */ >>> >>> Wouldn't it be better to avoid writing a lot of new allocation code >>> (with possibly subtle problems), and just use av_buffer_realloc() to >>> handle reallocation? (Possibly it could be moved to an internal >>> function to avoid the memcpy() required by realloc.) >> >> There are some differences that make most code in av_buffer_realloc() >> hard to be reused. >> In this one I'm using av_malloc instead of av_realloc, I update the >> AVBufferRef size with the requested size if it's equal or smaller than >> the available one and return doing nothing else instead of returning >> doing nothing at all only if the requested size is the exact same as the >> available one, I need to take precautions about what kind of buffer is >> passed to the function by properly freeing it using its callback then >> replacing it with default values, instead of knowing beforehand that the >> buffer was effectively created by av_buffer_realloc() itself where it >> can simply call av_realloc, etc. >> >> Trying to reuse code to follow all those details would make it harder to >> read and probably more prone to mistakes than just carefully writing the >> two separate functions doing the specific checks and allocations they >> require. > > Fine, if you say so. I'd probably argue we should still try to share > some code, since it duplicates all the allocation _and_ deallocation > code, only for the additional check to skip doing anything if the > underlying buffer is big enough. Anyway, I'm not blocking the patch > over this, and I see no technical issues in the patch. I admittedly did a lot of ricing here trying to reuse the AVBufferRef and AVBuffer structs, so if you think it's kinda risky then we could always instead just do something simpler/safer and slightly slower only if a bigger buffer is needed, like --------- ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel diff --git a/libavutil/buffer.c b/libavutil/buffer.c index 8d1aa5fa84..31237d1f5c 100644 --- a/libavutil/buffer.c +++ b/libavutil/buffer.c @@ -215,6 +215,38 @@ int av_buffer_realloc(AVBufferRef **pbuf, int size) return 0; } +static inline int buffer_fast_alloc(AVBufferRef **pbuf, int size, + AVBufferRef* (*buffer_alloc)(int size)) +{ + AVBufferRef *buf = *pbuf; + + if (buf && av_buffer_is_writable(buf) && + buf->data == buf->buffer->data && size <= buf->buffer->size) { + buf->size = FFMAX(0, size); + return 0; + } + + av_buffer_unref(pbuf); + + buf = buffer_alloc(size); + if (!buf) + return AVERROR(ENOMEM); + + *pbuf = buf; + + return 0; +} + +int av_buffer_fast_alloc(AVBufferRef **pbuf, int size) +{ + return buffer_fast_alloc(pbuf, size, av_buffer_alloc); +} + +int av_buffer_fast_allocz(AVBufferRef **pbuf, int size) +{ + return buffer_fast_alloc(pbuf, size, av_buffer_allocz); +} --------- Both options are fine with me. _______________________________________________ ffmpeg-devel mailing list