Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data

[James Almer <jamrial@gmail.com>]

On 5/27/2024 4:20 PM, Michael Niedermayer wrote:
> On Mon, May 27, 2024 at 03:17:15PM -0300, James Almer wrote:
>> On 5/27/2024 3:11 PM, Michael Niedermayer wrote:
>>> On Mon, May 27, 2024 at 10:15:43AM +0200, Anton Khirnov wrote:
>>>> Quoting Michael Niedermayer (2024-04-27 02:36:23)
>>>>> This allows detecting issues 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>
>>>>> ---
>>>>
>>>> I am against this patch. Checksumming side data is a fundamentally wrong
>>>> thing to do.
>>>
>>> It, or something equivalent is neccessary for regression testing.
>>> (and it was you who asked also for the tests i run to be part of
>>>    fate. But here you object to it)
>>>
>>> You know, not checking side data is not checking it so differences would then not be
>>> detected allowing for unintended changes to be introduced (aka bugs)
>>
>> You have seen how much code is needed to get hashing to work for all targets
>> with some types,
> 
>   framecrcenc.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 73 insertions(+), 3 deletions(-)
> 
> 70 more lines of code, in my patch
> 
> If we need another 70 to handle some corner cases, no idea if we do, thats
> still negligible

IAMF and video_enc_params. And potential future types. It's not negligible.

> 
> 
>> so it does feel like it's not the right thing to do.
> 
> I dont think i can follow that logic

Extra custom code to take into account specific side data types in order 
to output a common hash value. Do we really need to add that when we 
already have side data type specific parsing code to in ffprobe?

> 
> 
>> ffprobe (and f_sidedata) are what should be used for actual integrity
>> checks.
> 
> ffprobe cannot test ffmpeg, ffmpeg is a seperate excutable

I didn't say ffprobe should be used to test ffmpeg, i said ffprobe 
should be used to test side data integrity.

> 
> If you suggest that side data should not be tested in FFmpeg while packet.data
> should be tested. That position seems inconsistant to me
> 
> If you suggest that neither side data nor packet.data should be tested in FFmpeg
> iam confident that there would be a majority disagreeing.
> 
> f_sidedata is not at the output of ffmpeg so even if it could test it, it
> does not test the ffmpeg output.

I meant {a,f}_showinfo, sorry. And it does not apply to packet side 
data, obviously.

> We also dont replace running md5sum and framecrc on ffmpeg output by a bitstream
> filter.
> 
> Again, there is need to test what comes out of FFmpeg, thats at the muxer level
> thats what framecrcenc does.

Packet side data does not come out of ffmpeg, it comes out of 
libavformat (or libavcodec/libavfilter in the case of frame side data). 
A user of lavf can look at side data instead of needing a lavf muxer to 
print a hash that needs ad hoc code for it to match across targets.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".