From patchwork Sat Oct 27 19:41:00 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Thompson X-Patchwork-Id: 10802 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id C2B3C44D80B for ; Sat, 27 Oct 2018 22:41:06 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 30BA9689ECC; Sat, 27 Oct 2018 22:40:38 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 26164689A5A for ; Sat, 27 Oct 2018 22:40:32 +0300 (EEST) Received: by mail-wr1-f66.google.com with SMTP id d17-v6so4224253wre.11 for ; Sat, 27 Oct 2018 12:41:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jkqxz-net.20150623.gappssmtp.com; s=20150623; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=Mp42ZS4Q2nWvBkyr6KOaMDKVLfk66/ih5wjhBHzBqxI=; b=g4K905YfaEo+VGHZVOH+MsyHUBOaDkKZYJQdXcEVUOj/JfV/fDHXUQxgwGmBF0kGYN 3//evVYR/JEOxp8h9BfCq24iGuOzoKVkM+ovm46WbOs0KjLhWy2D9DUtgSwRZQP7mFJa naWbjjDD9XmbzKH4kt9oIV2buHZmVUcLLNVQSIvW87kmg2bwTVXThSSKxh0mv6fOG5mz C76nugGUv/Y6bVKsB4VsHkLAQOhYlpmQV4pyk7JOeHBtWxnyNzokNefgsfQcNOg3TWp5 43MxRNeV2bp+XUeFVdyxlRKyMt3YY9WaO0K9L5k40ui1KXkkEem9O+m4YvUG/PpoCsl+ LjtQ== 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=Mp42ZS4Q2nWvBkyr6KOaMDKVLfk66/ih5wjhBHzBqxI=; b=DorI/57K1CrPNVyY/m13Qv6c256W8mL0aQk1VQ1w5r3viyN0OyxhCIHFqWTnmm6UyM dbYiReZrPMiF1poBp6mo3Zfs8p6eNidwlV/vCh3m0gMYnhQpcLHMxM63bkMRCu5GcOC2 HspWZGE9IdfsQS08+IWuPzLbS1kg8dC3Fev5Bd6RyARE1r0nvswONoEDy8aMsZM3MTik AtIEHKzpAkpBXuGmKGgfJnhkesvA0VvzRFxvhGdZPD7sqF1A6VjsXUchCQQIB3w6mCGv O2zt0H/ytRadvQqSQzcQVqyix5IBpAkKdGqGjIRrVBR1jds+AhfOBh0VWz932zPG+SjF fLwg== X-Gm-Message-State: AGRZ1gIFPxpUfjaJOAgUTad1CjdmRBhuxyHIk12QATI89UVLJ/sAhtwS JHNfRf5BhTdm3FYaQxFqQGwd6u0+OUM= X-Google-Smtp-Source: AJdET5f68mYQAzvFDqGHCqeh8atELEbDJNaEQHlY82FKoXHAWfpWG9fP1hVik84Kwrtu6k98tNYC0g== X-Received: by 2002:adf:fac4:: with SMTP id a4-v6mr10091054wrs.202.1540669261489; Sat, 27 Oct 2018 12:41:01 -0700 (PDT) Received: from [192.168.0.4] (cpc91242-cmbg18-2-0-cust650.5-4.cable.virginm.net. [82.8.130.139]) by smtp.gmail.com with ESMTPSA id v189-v6sm853238wmd.40.2018.10.27.12.41.00 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 27 Oct 2018 12:41:00 -0700 (PDT) To: ffmpeg-devel@ffmpeg.org References: <20181026193708.8056-1-jamrial@gmail.com> <356433b4-7c74-29da-2f17-8b8cc71f71bc@jkqxz.net> <751b504c-6319-e780-cab4-cebbaee2e8de@gmail.com> From: Mark Thompson Message-ID: <946894f7-f649-dd98-2f24-65fa413ecd6c@jkqxz.net> Date: Sat, 27 Oct 2018 20:41:00 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <751b504c-6319-e780-cab4-cebbaee2e8de@gmail.com> Content-Language: en-US Subject: [FFmpeg-devel] [PATCH] cbs_vp9: Ensure that reserved zero bits are actually zero 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 27/10/18 20:26, James Almer wrote: > On 10/27/2018 4:13 PM, James Almer wrote: >> On 10/27/2018 4:11 PM, Mark Thompson wrote: >>> On 26/10/18 20:37, James Almer wrote: >>>> Signed-off-by: James Almer >>>> --- >>>> libavcodec/cbs_vp9_syntax_template.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/libavcodec/cbs_vp9_syntax_template.c b/libavcodec/cbs_vp9_syntax_template.c >>>> index 0db0f52a6d..b4a7f65e85 100644 >>>> --- a/libavcodec/cbs_vp9_syntax_template.c >>>> +++ b/libavcodec/cbs_vp9_syntax_template.c >>>> @@ -65,6 +65,7 @@ static int FUNC(color_config)(CodedBitstreamContext *ctx, RWContext *rw, >>>> if (profile == 1 || profile == 3) { >>>> infer(subsampling_x, 0); >>>> infer(subsampling_y, 0); >>>> + f(1, color_config_reserved_zero); >>>> } >>>> } >>>> >>>> >>> >>> This and the one above should probably be called "reserved_zero" for trace purposes. Not sure where the longer name came from. >>> >> >> Yeah, the spec simply says reserved_zero, so i'll change it. > > Looks like you came up with the longer name because there's another > "reserved_zero" field in the uncompressed header, and of course you > couldn't use it for both. > > The solution would be to move the color config fields to its own struct > called VP9RawColorConfig, like you did in cbs av1. Or this, so we don't store them at all :) libavcodec/cbs_vp9.c | 13 +++++++++++++ libavcodec/cbs_vp9.h | 2 -- libavcodec/cbs_vp9_syntax_template.c | 6 +++--- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/libavcodec/cbs_vp9.c b/libavcodec/cbs_vp9.c index 7498be4b73..c03ce986c0 100644 --- a/libavcodec/cbs_vp9.c +++ b/libavcodec/cbs_vp9.c @@ -314,6 +314,12 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc, current->name = prob; \ } while (0) +#define fixed(width, name, value) do { \ + av_unused uint32_t fixed_value = value; \ + CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \ + 0, &fixed_value, value, value)); \ + } while (0) + #define infer(name, value) do { \ current->name = value; \ } while (0) @@ -331,6 +337,7 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc, #undef fle #undef delta_q #undef prob +#undef fixed #undef infer #undef byte_alignment @@ -370,6 +377,11 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc, xf(8, name.prob, current->name, subs, __VA_ARGS__); \ } while (0) +#define fixed(width, name, value) do { \ + CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \ + 0, value, value, value)); \ + } while (0) + #define infer(name, value) do { \ if (current->name != (value)) { \ av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \ @@ -392,6 +404,7 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc, #undef fle #undef delta_q #undef prob +#undef fixed #undef infer #undef byte_alignment diff --git a/libavcodec/cbs_vp9.h b/libavcodec/cbs_vp9.h index 7eee6d5e9e..ac66da5af1 100644 --- a/libavcodec/cbs_vp9.h +++ b/libavcodec/cbs_vp9.h @@ -84,7 +84,6 @@ typedef struct VP9RawFrameHeader { uint8_t frame_marker; uint8_t profile_low_bit; uint8_t profile_high_bit; - uint8_t profile_reserved_zero; uint8_t show_existing_frame; uint8_t frame_to_show_map_idx; @@ -99,7 +98,6 @@ typedef struct VP9RawFrameHeader { uint8_t color_range; uint8_t subsampling_x; uint8_t subsampling_y; - uint8_t color_config_reserved_zero; uint8_t refresh_frame_flags; diff --git a/libavcodec/cbs_vp9_syntax_template.c b/libavcodec/cbs_vp9_syntax_template.c index cd5b83a4f5..ec8463e6d7 100644 --- a/libavcodec/cbs_vp9_syntax_template.c +++ b/libavcodec/cbs_vp9_syntax_template.c @@ -59,7 +59,7 @@ static int FUNC(color_config)(CodedBitstreamContext *ctx, RWContext *rw, if (profile == 1 || profile == 3) { f(1, subsampling_x); f(1, subsampling_y); - f(1, color_config_reserved_zero); + fixed(1, reserved_zero, 0); } else { infer(subsampling_x, 1); infer(subsampling_y, 1); @@ -69,7 +69,7 @@ static int FUNC(color_config)(CodedBitstreamContext *ctx, RWContext *rw, if (profile == 1 || profile == 3) { infer(subsampling_x, 0); infer(subsampling_y, 0); - f(1, color_config_reserved_zero); + fixed(1, reserved_zero, 0); } } @@ -278,7 +278,7 @@ static int FUNC(uncompressed_header)(CodedBitstreamContext *ctx, RWContext *rw, f(1, profile_high_bit); profile = (current->profile_high_bit << 1) + current->profile_low_bit; if (profile == 3) - f(1, profile_reserved_zero); + fixed(1, reserved_zero, 0); f(1, show_existing_frame); if (current->show_existing_frame) {