Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
From: James Almer <jamrial-at-gmail.com@ffmpeg.org>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: List types and checksums for for side data
Date: Sat, 7 Jun 2025 23:54:03 -0300
Message-ID: <81deebf3-9693-49e5-b14f-7863775b934e@gmail.com> (raw)
In-Reply-To: <20250518212323.GH29660@pb2>


[-- Attachment #1.1.1: Type: text/plain, Size: 5128 bytes --]

On 5/18/2025 6:23 PM, Michael Niedermayer wrote:
> On Sun, May 18, 2025 at 11:43:14AM -0300, James Almer wrote:
>> On 5/18/2025 9:25 AM, Michael Niedermayer wrote:
>>> This allows detecting changes and regressions in side data related code, same as what
>>> framecrc does for before already for packet data itself.
>>>
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>    libavformat/framecrcenc.c                   |  116 +-
>>>    tests/ref/fate/autorotate                   |    2 +-
>>>    tests/ref/fate/cover-art-mp3-id3v2-remux    |    2 +-
>>>    tests/ref/fate/ffmpeg-bsf-input             |   10 +-
>>>    tests/ref/fate/ffmpeg-spec-disposition      |    2 +-
>>>    tests/ref/fate/force_key_frames-source      |  784 ++++++------
>>>    tests/ref/fate/force_key_frames-source-drop |   34 +-
>>>    tests/ref/fate/force_key_frames-source-dup  | 1224 +++++++++----------
>>>    tests/ref/fate/gapless-mp3                  |    6 +-
>>>    tests/ref/fate/h264_redundant_pps-side_data |    2 +-
>>>    tests/ref/fate/id3v2-priv-remux             |    2 +-
>>>    tests/ref/fate/matroska-hdr10-plus-remux    |    2 +-
>>>    tests/ref/fate/matroska-ogg-opus-remux      |    2 +-
>>>    tests/ref/fate/matroska-opus-remux          |    2 +-
>>>    tests/ref/fate/matroska-vp8-alpha-remux     |   14 +-
>>>    tests/ref/fate/mov-cover-image              |    2 +-
>>>    tests/ref/fate/segment-mp4-to-ts            |  250 ++--
>>>    tests/ref/fate/shortest                     |  100 +-
>>>    tests/ref/fate/webm-hdr10-plus-remux        |    2 +-
>>>    tests/ref/fate/webm-webvtt-remux            |   24 +-
>>>    20 files changed, 1346 insertions(+), 1236 deletions(-)
>>>
>>> diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcenc.c
>>> index 2ba20f3aab4..96c2a82eb2a 100644
>>> --- a/libavformat/framecrcenc.c
>>> +++ b/libavformat/framecrcenc.c
>>> @@ -21,8 +21,11 @@
>>>    #include <inttypes.h>
>>> +#include "config.h"
>>>    #include "libavutil/adler32.h"
>>>    #include "libavutil/avstring.h"
>>> +#include "libavutil/intreadwrite.h"
>>> +#include "libavutil/hdr_dynamic_metadata.h"
>>>    #include "libavcodec/codec_id.h"
>>>    #include "libavcodec/codec_par.h"
>>> @@ -48,6 +51,20 @@ static int framecrc_write_header(struct AVFormatContext *s)
>>>        return ff_framehash_write_header(s);
>>>    }
>>> +static av_unused void inline bswap(char *buf, int offset, int size)
>>> +{
>>> +    if (size == 8) {
>>> +        uint64_t val = AV_RN64(buf + offset);
>>> +        AV_WN64(buf + offset, av_bswap64(val));
>>> +    } else if (size == 4) {
>>> +        uint32_t val = AV_RN32(buf + offset);
>>> +        AV_WN32(buf + offset, av_bswap32(val));
>>> +    } else if (size == 2) {
>>> +        uint16_t val = AV_RN16(buf + offset);
>>> +        AV_WN16(buf + offset, av_bswap16(val));
>>> +    }
>>> +}
>>> +
>>>    static int framecrc_write_packet(struct AVFormatContext *s, AVPacket *pkt)
>>>    {
>>>        uint32_t crc = av_adler32_update(0, pkt->data, pkt->size);
>>> @@ -58,11 +75,104 @@ static int framecrc_write_packet(struct AVFormatContext *s, AVPacket *pkt)
>>>        if (pkt->flags != AV_PKT_FLAG_KEY)
>>>            av_strlcatf(buf, sizeof(buf), ", F=0x%0X", pkt->flags);
>>>        if (pkt->side_data_elems) {
>>> +        int i;
>>>            av_strlcatf(buf, sizeof(buf), ", S=%d", pkt->side_data_elems);
>>> -        for (int i = 0; i < pkt->side_data_elems; i++) {
>>> -            av_strlcatf(buf, sizeof(buf), ", %8"SIZE_SPECIFIER,
>>> -                        pkt->side_data[i].size);
>>> +        for (i=0; i<pkt->side_data_elems; i++) {
>>> +            const AVPacketSideData *const sd = &pkt->side_data[i];
>>> +            const uint8_t *data = sd->data;
>>> +            uint32_t side_data_crc = 0;
>>> +
>>> +            switch (sd->type) {
>>
>> Wont this potentially introduce extra work for when we add new types?
> 
> no, it will be less work
> 
> because maintaining this in a seperate branch is the identical work
> but in addition its also the extra work due to the seperate branch.
> and it adds complexity if one wants to bisect.
> 
> Every hour i have to spend maintaining this or anything
> else in a seperate branch is an hour less i spend on the ffmpeg core
> repository.
You're seeing this the wrong way. You want this feature, and every day 
it's not upstreamed, it's potential extra work in your local repo. 
That's on you, same as other people probably have their own local repos 
or public forks they need to be kept in sync with extra personal changes 
that are not upstreamed.
The way this should be handled is: Does the project need it, or do 
developers want it? Because the extra work i talked about earlier is 
potentially adding a case to the switch for every new side data type 
added from now on, that you're putting on other developers' shoulders.

That said, I'm not against this in principle, but again, it will mean 
extra work when adding future side data types that encapsulate long 
elaborate structs.


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

  reply	other threads:[~2025-06-08  2:54 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-05-18 12:25 Michael Niedermayer
2025-05-18 12:40 ` Michael Niedermayer
2025-05-18 14:43 ` Andreas Rheinhardt
2025-05-18 19:51   ` Michael Niedermayer
2025-06-07 18:01     ` Michael Niedermayer
2025-05-18 14:43 ` James Almer
2025-05-18 21:23   ` Michael Niedermayer
2025-06-08  2:54     ` James Almer [this message]
2025-06-09 20:24       ` Michael Niedermayer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=81deebf3-9693-49e5-b14f-7863775b934e@gmail.com \
    --to=jamrial-at-gmail.com@ffmpeg.org \
    --cc=ffmpeg-devel@ffmpeg.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git