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 3F31047DDA for ; Fri, 27 Oct 2023 11:46:44 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 2E17C68CA61; Fri, 27 Oct 2023 14:46:42 +0300 (EEST) Received: from mail-qk1-f174.google.com (mail-qk1-f174.google.com [209.85.222.174]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 3947D68018A for ; Fri, 27 Oct 2023 14:46:35 +0300 (EEST) Received: by mail-qk1-f174.google.com with SMTP id af79cd13be357-778711ee748so155803085a.2 for ; Fri, 27 Oct 2023 04:46:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1698407193; x=1699011993; darn=ffmpeg.org; h=content-transfer-encoding:in-reply-to:content-language:references :to:subject:user-agent:mime-version:date:message-id:from:from:to:cc :subject:date:message-id:reply-to; bh=Azrfuae6c7nkK8KQuoVNKHv7zMq0VujotWbtdIBbbwI=; b=mw/jV5M2y3+aJPu8dKLgGirbeb/Loy9vCDnpVXUjV3OMr2w7ulCH/QQ/ygFS//2OK0 tVOg4pegi31TTtAbL6hIueQF9dDvkbW+qELqz8nCV5FdpzvAtW4q1v9mSKsht0O68tdR C2vUT7ZRXeau22KgNJ9V6F2gxVfVGwAJMxgZkhqcX71yEdU34v8vq1tymYiz6xXEUMlr zIiaAIOvXSLVJ4skGsB8DQgH49OEvDGoSVSLW4XgvzvAm1RKQXeMKxMdb3PsajC0/HpZ HjQKrY1P2W6CPKd8BPXREbS3PyVDejk+8L29iW2iEsXIPAZCgvoRVn9ixysewXKQJRLH 4DTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698407193; x=1699011993; h=content-transfer-encoding:in-reply-to:content-language:references :to:subject:user-agent:mime-version:date:message-id:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Azrfuae6c7nkK8KQuoVNKHv7zMq0VujotWbtdIBbbwI=; b=TmmKBExvZee8lo9MHGNYLrz2sB0ehsrgD3aOH6OIBVfYwGKf0JboGFFN0AlJGwILnT V26DBGrH4HxUCbvwRJNa4ADn4ojbQ8MIJ1mi2GKUrO5Qg9nRZwY5aTnVjP6WFjV5mZM2 bNx+fwqZy0n4N5AkQBxK051Erz/EHJSutOsOJFlTV8bo44mb7loQGz14V3Xre6tZQPAO yT3GthhRTs88YX5JbxFleK107hh4lUjXXY7nKswPBXz7TrDC16SlRHRZPppubm2eBBqq pNonu47Lsds9jlmkv6uDP/alS9eOPxY2kbaMyVOLMQGokloa/+9E+ZmVKm2Q2sODKcEA 3Ulw== X-Gm-Message-State: AOJu0Yzge9v+VO/pp50+Q3pg+u3GTZ4cmU8G1eG0DyH23qese0cRf4BM 7FM+LFHMwtDST0MlGMI4MLqLDELj51o= X-Google-Smtp-Source: AGHT+IEUMGGRKpLXkcplBeZ+y0RFuJtVHykAGwPkkK8H5Hp+anQCvcANUFynmbDIjmzrg2Ks8W1qag== X-Received: by 2002:ae9:f719:0:b0:76c:8d5f:5954 with SMTP id s25-20020ae9f719000000b0076c8d5f5954mr2012745qkg.70.1698407192535; Fri, 27 Oct 2023 04:46:32 -0700 (PDT) Received: from [10.66.219.120] ([146.70.198.174]) by smtp.gmail.com with ESMTPSA id pa39-20020a05620a832700b0076f058f5834sm460358qkn.61.2023.10.27.04.46.31 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 27 Oct 2023 04:46:32 -0700 (PDT) From: quietvoid X-Google-Original-From: quietvoid Message-ID: <43bfe941-5150-4692-bb6f-170b1c0ec833@gmail.com> Date: Fri, 27 Oct 2023 07:46:31 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: ffmpeg-devel@ffmpeg.org References: <20230809174657.76-1-tcChlisop0@gmail.com> <20231026214400.GM3543730@pb2> Content-Language: en-US In-Reply-To: <20231026214400.GM3543730@pb2> Subject: Re: [FFmpeg-devel] [PATCH v3] avcodec/dovi_rpu: verify RPU data CRC32 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On 26/10/2023 17.44, Michael Niedermayer wrote: > On Wed, Aug 09, 2023 at 01:46:57PM -0400, quietvoid wrote: >> The Dolby Vision RPU contains a CRC32 to validate the payload against. >> The implementation is CRC32/MPEG-2. >> >> The CRC is only verified with the AV_EF_CRCCHECK flag. >> >> Signed-off-by: quietvoid >> --- >> libavcodec/dovi_rpu.c | 46 ++++++++++++++++++++++++++++++++++++++++--- >> libavcodec/dovi_rpu.h | 3 ++- >> libavcodec/hevcdec.c | 3 ++- >> 3 files changed, 47 insertions(+), 5 deletions(-) >> >> diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c >> index dd38936552..1dfeee7564 100644 >> --- a/libavcodec/dovi_rpu.c >> +++ b/libavcodec/dovi_rpu.c >> @@ -22,6 +22,7 @@ >> */ >> >> #include "libavutil/buffer.h" >> +#include "libavutil/crc.h" >> >> #include "dovi_rpu.h" >> #include "golomb.h" >> @@ -191,13 +192,17 @@ static inline int64_t get_se_coef(GetBitContext *gb, const AVDOVIRpuDataHeader * >> } \ >> } while (0) >> >> -int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) >> +int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size, >> + int err_recognition) >> { >> AVDOVIRpuDataHeader *hdr = &s->header; >> GetBitContext *gb = &(GetBitContext){0}; >> DOVIVdrRef *vdr; >> int ret; >> >> + size_t actual_rpu_size; >> + uint8_t trailing_zeroes = 0; >> + >> uint8_t nal_prefix; >> uint8_t rpu_type; >> uint8_t vdr_seq_info_present; >> @@ -205,7 +210,22 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) >> uint8_t use_prev_vdr_rpu; >> uint8_t use_nlq; >> uint8_t profile; >> - if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) >> + >> + uint32_t rpu_data_crc32; >> + uint32_t computed_crc32; >> + >> + for (int i = rpu_size - 1; i > 0; i--) { >> + if (!rpu[i]) { >> + trailing_zeroes++; >> + } else { >> + break; >> + } >> + } >> + >> + actual_rpu_size = rpu_size - trailing_zeroes; >> + >> + /* Exclude trailing byte (0x80) from reader */ >> + if ((ret = init_get_bits8(gb, rpu, actual_rpu_size - 1)) < 0) >> return ret; >> >> /* RPU header, common values */ >> @@ -440,7 +460,27 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) >> color->source_diagonal = get_bits(gb, 10); >> } >> >> - /* FIXME: verify CRC32, requires implementation of AV_CRC_32_MPEG_2 */ >> + if (!(err_recognition & AV_EF_CRCCHECK)) >> + return 0; >> + >> + /* Skip unsupported until CRC32 */ >> + skip_bits_long(gb, get_bits_left(gb) - 32); >> + >> + rpu_data_crc32 = get_bits_long(gb, 32); >> + >> + /* Verify CRC32, buffer excludes the prefix, CRC32 and trailing byte */ >> + computed_crc32 = av_bswap32(av_crc(av_crc_get_table(AV_CRC_32_IEEE), >> + -1, rpu + 1, actual_rpu_size - 6)); >> + >> + if (rpu_data_crc32 != computed_crc32) { >> + av_log(s->logctx, AV_LOG_ERROR, >> + "RPU CRC mismatch! Expected %"PRIu32", received %"PRIu32"\n", >> + rpu_data_crc32, computed_crc32); >> + >> + if (err_recognition & AV_EF_EXPLODE) >> + goto fail; >> + } > (correctly designed) CRCs have the beautifull symmetry that you can merge > the crc32 value into the crc computation and then a 0 means no CRC missmatch > (there are many other cool properties but this one allows to simplify the code) > > This works too: (and is simpler) > > /* Skip unsupported until CRC32 */ > skip_bits_long(gb, get_bits_left(gb)); > > /* Verify CRC32, buffer excludes the prefix, CRC32 and trailing byte */ > computed_crc32 = av_bswap32(av_crc(av_crc_get_table(AV_CRC_32_IEEE), > -1, rpu + 1, actual_rpu_size - 2)); > > if (computed_crc32) { > av_log(s->logctx, AV_LOG_ERROR, "RPU CRC mismatch! %"PRIX32"\n", > computed_crc32); > > if (err_recognition & AV_EF_EXPLODE) > goto fail; > } Hi Michael. I like the idea and it's a cool property. However the then printed CRC on mismatch is not a useful value, so I'm unsure if it's better to simplify here. I like having the expected CRC logged here. Thanks. _______________________________________________ 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".