Re: [FFmpeg-devel] 回复: [PATCH v1 09/19] avcodec/h274: add ff_h274_hash functions

[Nuo Mi <nuomi2021@gmail.com>]

On Fri, May 9, 2025 at 2:37 AM Wu Jianhua <toqsxw@outlook.com> wrote:

> Andreas Rheinhardt:
> > toqsxw@gmail.com:
> >> From: Wu Jianhua <toqsxw@outlook.com>
> >>
> >> Signed-off-by: Wu Jianhua <toqsxw@outlook.com>
> >> ---
> >>  libavcodec/h274.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  libavcodec/h274.h |   7 ++
> >>  2 files changed, 170 insertions(+)
> >>
> >> diff --git a/libavcodec/h274.c b/libavcodec/h274.c
> >> index 5709200322..9edc705042 100644
> >> --- a/libavcodec/h274.c
> >> +++ b/libavcodec/h274.c
> >> @@ -26,7 +26,11 @@
> >>   */
> >>
> >>  #include "libavutil/avassert.h"
> >> +#include "libavutil/bswap.h"
> >> +#include "libavutil/crc.h"
> >>  #include "libavutil/imgutils.h"
> >> +#include "libavutil/md5.h"
> >> +#include "libavutil/mem.h"
> >>
> >>  #include "h274.h"
> >>
> >> @@ -790,3 +794,162 @@ static const int8_t R64T[64][64] = {
> >>           17, -16,  15, -14,  13, -12,  11, -10,   9,  -8,   7,  -6,
>  4,  -3,   2,  -1,
> >>      }
> >>  };
> >> +
> >> +static int verify_plane_md5(struct AVMD5 *ctx,
> >> +    const uint8_t *src, const int w, const int h, const int stride,
> >> +    const uint8_t *expected)
> >> +{
> >> +#define MD5_SIZE 16
> >> +    uint8_t md5[MD5_SIZE];
> >> +    av_md5_init(ctx);
> >> +    for (int j = 0; j < h; j++) {
> >> +        av_md5_update(ctx, src, w);
> >> +        src += stride;
> >> +    }
> >> +    av_md5_final(ctx, md5);
> >> +
> >> +    if (memcmp(md5, expected, MD5_SIZE))
> >> +        return AVERROR_INVALIDDATA;
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int verify_plane_crc(const uint8_t *src, const int w, const int
> h, const int stride,
> >> +    const uint8_t *expected)
> >> +{
> >> +    uint32_t crc = 0x0F1D;     // CRC-16-CCITT-AUG
> >> +    const AVCRC *ctx = av_crc_get_table(AV_CRC_16_CCITT);
> >> +
> >> +    for (int j = 0; j < h; j++) {
> >> +        crc = av_crc(ctx, crc, src, w);
> >> +        src += stride;
> >> +    }
> >> +    crc = av_bswap16(crc);
> >> +
> >> +    if (memcmp(&crc, expected, sizeof(uint16_t)))
> >
> > This seems endian-dependent (as are the other checks). Why don't you
> > simply pass the proper CRC as uint16_t/uint32_t parameter?
> >
> >> +        return AVERROR_INVALIDDATA;
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +#define CAL_CHECKSUM(pixel) (checksum + ((pixel) ^ xor_mask))
> >> +static int verify_plane_checksum(const uint8_t *src, const int w,
> const int h, const int stride, const int ps,
> >> +    const uint8_t *expected)
> >> +{
> >> +    uint32_t checksum = 0;
> >> +
> >> +    for (int y = 0; y < h; y++) {
> >> +        for (int x = 0; x < w; x++) {
> >> +            const int xor_mask = (x & 0xFF) ^ (y & 0xFF) ^ (x >> 8) ^
> (y >> 8);
> >> +            checksum = CAL_CHECKSUM(src[x << ps]);
> >> +            if (ps)
> >> +                checksum = CAL_CHECKSUM(src[(x << ps) + 1]);
> >
> > Why don't you use checksum += instead of using checksum in the
> > CAL_CHECKSUM macro in a hardcoded manner?
> >
> >> +        }
> >> +        src += stride;
> >> +    }
> >> +
> >> +    if (checksum != *(uint32_t*)expected)
> >> +        return AVERROR_INVALIDDATA;
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static const uint8_t *get_plane_hash(const H274SEIPictureHash *h,
> const int plane)
> >> +{
> >> +    if (!h->hash_type)
> >> +        return h->md5[plane];
> >> +    if (h->hash_type == 1)
> >> +        return (uint8_t*)&h->crc[plane];
> >> +    return (uint8_t*)&h->checksum[plane];
> >> +}
> >
> > This function seems completely useless; you should better pass the
> > proper expected value to the functions directly (by value for non-md5).
> >
> >> +
> >> +enum {
> >> +    HASH_MD5SUM,
> >> +    HASH_CRC,
> >> +    HASH_CHECKSUM,
> >> +    HASH_LAST = HASH_CHECKSUM,
> >> +};
> >> +
> >> +struct H274HashContext {
> >> +    int type;
> >> +    struct AVMD5 *ctx;
> >> +};
> >> +
> >> +int ff_h274_hash_freep(H274HashContext **ctx)
> >> +{
> >> +    if (ctx && *ctx) {
> >
> > Don't check for ctx. Passing a NULL to a freep-function is insane.
> >
> >> +        H274HashContext *c = *ctx;
> >> +        if (c->ctx)
> >> +            av_free(c->ctx);
> >> +        av_freep(ctx);
> >> +    }
> >> +    return 0;
> >
> > Why is this function returning anything?
> >
> >> +}
> >> +
> >> +int ff_h274_hash_init(H274HashContext **ctx, const int type)
> >> +{
> >> +    H274HashContext *c;
> >> +
> >> +    if (type > HASH_LAST || !ctx)
> >> +        return AVERROR(EINVAL);
> >> +
> >> +    c = *ctx;
> >> +    if (c) {
> >> +        if (c->type != type) {
> >> +            if (c->type == HASH_MD5SUM)
> >> +                av_freep(&c->ctx);
> >> +            c->type = type;
> >> +        }
> >> +    } else {
> >> +        *ctx = c = av_mallocz(sizeof(H274HashContext));
> >
> > Why is this separately allocated?
> >
> >> +        if (!c)
> >> +            return AVERROR(ENOMEM);
> >> +        c->type = type;
> >> +    }
> >> +
> >> +    if (type == HASH_MD5SUM && !c->ctx) {
> >> +        c->ctx = av_md5_alloc();
> >> +        if (!c->ctx)
> >> +            return AVERROR(ENOMEM);
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +int ff_h274_hash_verify(H274HashContext *c, const H274SEIPictureHash
> *hash,
> >> +    const AVFrame *frame, const int coded_width, const int
> coded_height)
> >> +{
> >> +    const AVPixFmtDescriptor *desc;
> >> +    int err = 0;
> >> +
> >> +    if (!c || !hash || !frame)
> >> +        return AVERROR(EINVAL);
> >> +
> >> +    if (c->type != hash->hash_type)
> >> +        return AVERROR(EINVAL);
> >> +
> >> +    desc = av_pix_fmt_desc_get(frame->format);
> >> +    if (!desc)
> >> +        return AVERROR(EINVAL);
> >> +
> >> +    for (int i = 0; i < desc->nb_components; i++) {
> >> +        const int w             = i ? (coded_width  >>
> desc->log2_chroma_w) : coded_width;
> >> +        const int h             = i ? (coded_height >>
> desc->log2_chroma_h) : coded_height;
> >> +        const int ps            = desc->comp[i].step - 1;
> >> +        const uint8_t *expected = get_plane_hash(hash, i);
> >> +        const uint8_t *src      = frame->data[i];
> >> +        const int stride        = frame->linesize[i];
> >> +
> >> +        if (c->type == HASH_MD5SUM)
> >> +            err = verify_plane_md5(c->ctx, src, w << ps, h, stride,
> expected);
> >> +        else if (c->type == HASH_CRC)
> >> +            err = verify_plane_crc(src, w << ps, h, stride, expected);
> >> +        else if (c->type == HASH_CHECKSUM)
> >> +            err = verify_plane_checksum(src, w, h, stride, ps,
> expected);
> >> +        if (err < 0)
> >> +            goto fail;
> >> +    }
> >> +
> >> +fail:
> >> +    return err;
> >> +}
> >> diff --git a/libavcodec/h274.h b/libavcodec/h274.h
> >> index e1803edaf3..e2bb1aec55 100644
> >> --- a/libavcodec/h274.h
> >> +++ b/libavcodec/h274.h
> >> @@ -64,6 +64,8 @@ int ff_h274_apply_film_grain(AVFrame *out, const
> AVFrame *in,
> >>                               H274FilmGrainDatabase *db,
> >>                               const AVFilmGrainParams *params);
> >>
> >> +typedef struct H274HashContext H274HashContext;
> >> +
> >>  typedef struct H274SEIPictureHash {
> >>      int present;
> >>      union {
> >> @@ -74,4 +76,9 @@ typedef struct H274SEIPictureHash {
> >>      uint8_t hash_type;
> >>  } H274SEIPictureHash;
> >>
> >> +int ff_h274_hash_init(H274HashContext **c, int type);
> >> +int ff_h274_hash_verify(H274HashContext *c, const H274SEIPictureHash
> *hash,
> >> +    const AVFrame *frame, int coded_width, int coded_height);
> >> +int ff_h274_hash_freep(H274HashContext **c);
> >> +
> >>  #endif /* AVCODEC_H274_H */
>
> Hi Andreas,
>
> Thanks for your reviews! They help a lot. I've updated the v2
> patchset based on your comments. Would you help to review the
> v2 patches?
>
Hi Andreas,
Thank you for the review.

Hi Andreas and all,
I will push this next week unless there are any objections.

Thank you.

>
> Thanks,
> Jianhua
> _______________________________________________
> 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".
>
_______________________________________________
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".