From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 80632C3A5A2 for ; Tue, 3 Sep 2019 09:56:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5CE1A22DBF for ; Tue, 3 Sep 2019 09:56:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728625AbfICJ4s (ORCPT ); Tue, 3 Sep 2019 05:56:48 -0400 Received: from metis.ext.pengutronix.de ([85.220.165.71]:54727 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726631AbfICJ4r (ORCPT ); Tue, 3 Sep 2019 05:56:47 -0400 Received: from lupine.hi.pengutronix.de ([2001:67c:670:100:3ad5:47ff:feaf:1a17] helo=lupine) by metis.ext.pengutronix.de with esmtp (Exim 4.92) (envelope-from ) id 1i55YG-0000fO-Qa; Tue, 03 Sep 2019 11:56:40 +0200 Message-ID: <1567504599.5229.1.camel@pengutronix.de> Subject: Re: [PATCH 02/12] media: hantro: Do not reorder H264 scaling list From: Philipp Zabel To: Jonas Karlman , Ezequiel Garcia Cc: Mauro Carvalho Chehab , Hans Verkuil , Boris Brezillon , Paul Kocialkowski , "linux-media@vger.kernel.org" , "linux-rockchip@lists.infradead.org" , "linux-kernel@vger.kernel.org" Date: Tue, 03 Sep 2019 11:56:39 +0200 In-Reply-To: References: <20190901124531.23645-1-jonas@kwiboo.se> <1567432843.3666.6.camel@pengutronix.de> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6-1+deb9u2 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 2001:67c:670:100:3ad5:47ff:feaf:1a17 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2019-09-02 at 16:18 +0000, Jonas Karlman wrote: > On 2019-09-02 16:00, Philipp Zabel wrote: > > Hi Jonas, > > > > On Sun, 2019-09-01 at 12:45 +0000, Jonas Karlman wrote: > > > Scaling list supplied from userspace using ffmpeg and libva-v4l2-request > > > is already in matrix order and can be used without applying the inverse > > > scanning process. > > > > "in matrix order" is equivalent to "in raster scan order"? > > The values supplied by ffmpeg and libva-v4l2-request is in the order after the > inverse scanning process has been applied (scaling list has been transformed > into a scaling matrix). Not sure what this is called, "matrix order" seemed > close enough. Ok, after reading chapters 8.5.6 Inverse scanning process for 4x4 transform coefficients and scaling lists 8.5.7 Inverse scanning process for 8x8 transform coefficients and scaling lists of ITU-T Rec. H.264, this seems clear enough. I just asked to make sure, because libva documentation uses the term "raster scan" [1]. [1] http://intel.github.io/libva/structVAIQMatrixBufferH264.html > Since there is two scan orders, zig-zag and field, and cedrus already expecting > the values in "matrix" order, it seems more logical to let userspace handle the > inverse scanning process. I agree. [...] > > This changes the size of struct hantro_h264_dec_priv_tbl. Did this > > describe the auxiliary buffer format incorrectly before? > > Based on RKMPP and Hantro SDK the HW expects the 8x8 inter/intra list for > Y-component to be located at indices 0 and 1, lists for Cr/Cb is only used for > 4:4:4 and HW only supports 4:0:0/4:2:0 if I am not mistaken. So the unused > extra 4 lists at the end of the auxiliary buffer seemed like a waste, > also RKMPP and Hantro SDK only seemed to allocate space for 2 lists. Ok. > > After this change, the second 8x8 scaling list has moved to a different > > offset. Is this where the hardware has always been looking for it, or is > > there a change missing in another place? > > As mentioned above HW only looks at indices 0 and 1, and ffmpeg will store the > inter/intra Y list at indices 0 and 3 as seen at [1], in similar way cedrus only > use indices 0 and 3 at [2]. > FFmpeg memcpy entire scaling_matrix8 to scaling_list_8x8 for v4l2-request-api > and memcpy scaling_matrix8[0] and scaling_matrix8[3] for vaapi. > > You can see the effect of this patch using the h264_tivo_sample.ts sample from > cover letter, patch 3-8 must be applied. With this patch applied the green > football field will stay green, without the patch the field will shift in colors. > > [1] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/h264_ps.c#L299-L308 > [2] https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/sunxi/cedrus/cedrus_h264.c#n231 Thank you, I'll try this. regards Philipp From mboxrd@z Thu Jan 1 00:00:00 1970 From: Philipp Zabel Subject: Re: [PATCH 02/12] media: hantro: Do not reorder H264 scaling list Date: Tue, 03 Sep 2019 11:56:39 +0200 Message-ID: <1567504599.5229.1.camel@pengutronix.de> References: <20190901124531.23645-1-jonas@kwiboo.se> <1567432843.3666.6.camel@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Jonas Karlman , Ezequiel Garcia Cc: Mauro Carvalho Chehab , Hans Verkuil , Boris Brezillon , Paul Kocialkowski , "linux-media@vger.kernel.org" , "linux-rockchip@lists.infradead.org" , "linux-kernel@vger.kernel.org" List-Id: linux-rockchip.vger.kernel.org On Mon, 2019-09-02 at 16:18 +0000, Jonas Karlman wrote: > On 2019-09-02 16:00, Philipp Zabel wrote: > > Hi Jonas, > > > > On Sun, 2019-09-01 at 12:45 +0000, Jonas Karlman wrote: > > > Scaling list supplied from userspace using ffmpeg and libva-v4l2-request > > > is already in matrix order and can be used without applying the inverse > > > scanning process. > > > > "in matrix order" is equivalent to "in raster scan order"? > > The values supplied by ffmpeg and libva-v4l2-request is in the order after the > inverse scanning process has been applied (scaling list has been transformed > into a scaling matrix). Not sure what this is called, "matrix order" seemed > close enough. Ok, after reading chapters 8.5.6 Inverse scanning process for 4x4 transform coefficients and scaling lists 8.5.7 Inverse scanning process for 8x8 transform coefficients and scaling lists of ITU-T Rec. H.264, this seems clear enough. I just asked to make sure, because libva documentation uses the term "raster scan" [1]. [1] http://intel.github.io/libva/structVAIQMatrixBufferH264.html > Since there is two scan orders, zig-zag and field, and cedrus already expecting > the values in "matrix" order, it seems more logical to let userspace handle the > inverse scanning process. I agree. [...] > > This changes the size of struct hantro_h264_dec_priv_tbl. Did this > > describe the auxiliary buffer format incorrectly before? > > Based on RKMPP and Hantro SDK the HW expects the 8x8 inter/intra list for > Y-component to be located at indices 0 and 1, lists for Cr/Cb is only used for > 4:4:4 and HW only supports 4:0:0/4:2:0 if I am not mistaken. So the unused > extra 4 lists at the end of the auxiliary buffer seemed like a waste, > also RKMPP and Hantro SDK only seemed to allocate space for 2 lists. Ok. > > After this change, the second 8x8 scaling list has moved to a different > > offset. Is this where the hardware has always been looking for it, or is > > there a change missing in another place? > > As mentioned above HW only looks at indices 0 and 1, and ffmpeg will store the > inter/intra Y list at indices 0 and 3 as seen at [1], in similar way cedrus only > use indices 0 and 3 at [2]. > FFmpeg memcpy entire scaling_matrix8 to scaling_list_8x8 for v4l2-request-api > and memcpy scaling_matrix8[0] and scaling_matrix8[3] for vaapi. > > You can see the effect of this patch using the h264_tivo_sample.ts sample from > cover letter, patch 3-8 must be applied. With this patch applied the green > football field will stay green, without the patch the field will shift in colors. > > [1] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/h264_ps.c#L299-L308 > [2] https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/sunxi/cedrus/cedrus_h264.c#n231 Thank you, I'll try this. regards Philipp