On Sat, May 24, 2025 at 10:02 PM Nuo Mi <nuomi2...@gmail.com> wrote:
>
>
> On Fri, May 9, 2025 at 2:37 AM Wu Jianhua <toq...@outlook.com> wrote:
>
>> Andreas Rheinhardt:
>> > toq...@gmail.com:
>> >> From: Wu Jianhua <toq...@outlook.com>
>> >>
>> >> Signed-off-by: Wu Jianhua <toq...@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.
>
Applied.
Thanks
>
> 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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
