From patchwork Fri Feb 2 06:41:17 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Sakamoto X-Patchwork-Id: 10196263 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 08F5C603D7 for ; Fri, 2 Feb 2018 06:41:34 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DBFDB28DE3 for ; Fri, 2 Feb 2018 06:41:33 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id CFEC528DEB; Fri, 2 Feb 2018 06:41:33 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4A17C28DE3 for ; Fri, 2 Feb 2018 06:41:30 +0000 (UTC) Received: from alsa0.perex.cz (localhost [127.0.0.1]) by alsa0.perex.cz (Postfix) with ESMTP id C786826728A; Fri, 2 Feb 2018 07:41:28 +0100 (CET) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa0.perex.cz (Postfix, from userid 1000) id 6996526728A; Fri, 2 Feb 2018 07:41:27 +0100 (CET) Received: from smtp-proxy002.phy.lolipop.jp (smtp-proxy002.phy.lolipop.jp [157.7.104.43]) by alsa0.perex.cz (Postfix) with ESMTP id 8E639266F64; Fri, 2 Feb 2018 07:41:20 +0100 (CET) Received: from smtp-proxy002.phy.lolipop.lan (HELO smtp-proxy002.phy.lolipop.jp) (172.19.44.43) (smtp-auth username m12129643-o-takashi , mechanism plain) by smtp-proxy002.phy.lolipop.jp (qpsmtpd/0.82) with ESMTPA; Fri, 02 Feb 2018 15:41:17 +0900 Received: from 127.0.0.1 (127.0.0.1) by smtp-proxy002.phy.lolipop.jp (LOLIPOP-Fsecure); Fri, 02 Feb 2018 15:41:17 +0900 (JST) X-Virus-Status: clean(LOLIPOP-Fsecure) To: furrywolf , patch@alsa-project.org References: <1517508732-27545-1-git-send-email-alsa2@bushytails.net> From: Takashi Sakamoto Message-ID: <7892ed61-e1fc-47ac-406e-dfeba468f9b7@sakamocchi.jp> Date: Fri, 2 Feb 2018 15:41:17 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <1517508732-27545-1-git-send-email-alsa2@bushytails.net> Content-Language: en-US Cc: alsa-devel@alsa-project.org Subject: Re: [alsa-devel] [PATCH - alsa-lib 1/1] Patch for another bug in snd_pcm_area_silence(). X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org X-Virus-Scanned: ClamAV using ClamSMTP Hi, On Feb 2 2018 03:12, furrywolf wrote: > Only silence areas 64 bits at a time if it's possible to do so, which is > when either the silence values are all zero, or when the format width > divides evenly into 64 bits. For formats that are neither of these, let > the width-specific code handle the entire silencing. Silencing formats > that are not evenly divisible into 64 bits in 64-bit chunks, when the > data isn't just zeroes, results in values being written to shifting > positions in the sample, giving garbage. > > Makes Takashi Sakamoto's tester happy for all tests. Yay! (And Thanks!) > > Signed-off-by: furrywolf > > diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c > index 1753cda..5f9bd9f 100644 > --- a/src/pcm/pcm.c > +++ b/src/pcm/pcm.c > @@ -2947,7 +2947,7 @@ int snd_pcm_area_silence(const snd_pcm_channel_area_t *dst_area, snd_pcm_uframes > dst = snd_pcm_channel_area_addr(dst_area, dst_offset); > width = snd_pcm_format_physical_width(format); > silence = snd_pcm_format_silence_64(format); > - if (dst_area->step == (unsigned int) width) { > + if (dst_area->step == (unsigned int) width && (!silence || !(64 % width))) { > unsigned int dwords = samples * width / 64; > uint64_t *dstp = (uint64_t *)dst; > samples -= dwords * 64 / width; A condition of '!silence' seems to skip cases of 'signed' format of PCM samples; e.g. S16, because 'snd_pcm_format_silence_64()' returns 0 as silent data for these samples. However, the above block of code is a fast path for data samples which are 'divisors of 64 bit'. In the code block, silent data is handled as 64 bit variable to be copied to given buffer iteratively. In my understanding, the condition is not appropriate to the code block. On the other hand, a condition of '!(64 % width)' is appropriate. If any 24 bit data sample is handled in the block, copied silent data aligned to 64 bit include invalid 8 bits. For example of 'U24_3LE': Given buffer, aligned 64 bit: 0x'xxxx'xxxx'xxxx'xxxx'yyyy'yyyy'yyyy'yyyy'zzzz'zzzz'zzzz'zzzz Silent data for 'U24_3LE' returned from 'snd_pcm_format_silence_64(): 0x'0000'8000'0080'0000ull (little endian) As a result of the fast path: 0x'xxxx'xxxx'xxxx'xxxx'yyyy'yyyy'yyyy'yyyy'zzzz'zzzz'zzzz'zzzz v v 0x'0000'8000'0080'0000'0000'8000'0080'0000'0000'8000'0080'0000 (sample data with 'x-z' represent data aligned to 64 bits) However, expected: 0x'8000'0080'0000'8000'0080'0000'8000'0080'0000'8000'0080'0000 0x'gggg'gghh'hhhh'iiii'iijj'jjjj'kkkk'kkll'llll'mmmm'mmnn'nnnn (sample data with 'g-n' represent data aligned to 24 bit) Practically, for any 24 bit sample data, we didn't see any issue caused by the above mechanism because in most case 'signed' sample data is handles by usual use cases, in my opinion. Of course, a condition of '!silence' is a good optimization for cases of 'signed' PCM samples. But it's not good for the other kind of data format (e.g. DSD). At present, there's no sample format which has 24 bit data for non-PCM format, however it's better to avoid future problem, unless we were astrologers. Overall, the fast path is not designed to handle any 24 bit sample data. We _should_ skip the fast path for any 24 bit samples, regardless of sign. Thus, below patch is enough for an issue to which I addressed in my previous message[1]. I can pass my test[2] with this patch. samples -= dwords * 64 / width; ======== 8< -------- From bfb44ca7e13d794478c0cee1a1abf96d4b664d63 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Fri, 2 Feb 2018 14:44:35 +0900 Subject: [PATCH] pcm: fix a bug to copy silent samples aligned to 64 bits for 24 bit sample cases A function of 'snd_pcm_area_silence()' has a fast path to copy silent data efficiently. However, the fast path works well just for a case that target buffer consists of data samples for which unit of data alignment is divisors of 64 bits. At present, the fast path handles sample data aligned to 24 bit. In this case, the buffer can includes extra 8 bits. This has no issue for 'signed' case because silent data is zero, however it has an issue for 'unsigned' case. This commit fixes the bug by skipping cases of sample data of 24 bit. Signed-off-by: Takashi Sakamoto --- src/pcm/pcm.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c index 1753cdac..ddd67975 100644 --- a/src/pcm/pcm.c +++ b/src/pcm/pcm.c @@ -2947,7 +2947,12 @@ int snd_pcm_area_silence(const snd_pcm_channel_area_t *dst_area, snd_pcm_uframes dst = snd_pcm_channel_area_addr(dst_area, dst_offset); width = snd_pcm_format_physical_width(format); silence = snd_pcm_format_silence_64(format); - if (dst_area->step == (unsigned int) width) { + + /* + * Iterate copying silent sample for sample data aligned to 64 bit. + * This is a fast path. + */ + if (dst_area->step == (unsigned int) width && 64 % width == 0) { unsigned int dwords = samples * width / 64; uint64_t *dstp = (uint64_t *)dst;