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

[James Almer]

On 5/27/2024 4:50 PM, Michael Niedermayer wrote:
> On Mon, May 27, 2024 at 04:33:21PM -0300, James Almer wrote:
>> On 5/27/2024 4:31 PM, Michael Niedermayer wrote:
>>> On Mon, May 27, 2024 at 09:20:55PM +0200, 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 at 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
>>>>
>>>>
>>>>> so it does feel like it's not the right thing to do.
>>>>
>>>> I dont think i can follow that logic
>>>>
>>>>
>>>>> ffprobe (and f_sidedata) are what should be used for actual integrity
>>>>> checks.
>>>>
>>>> ffprobe cannot test ffmpeg, ffmpeg is a seperate excutable
>>>>
>>>> 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.
>>>> 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.
>>>
>>> There is also an additional aspect
>>> and that is efficiency or "time taken by all fate tests"
>>> framecrcenc already has all the side data, it costs basically 0 time to print that
>>>
>>> any ffprobe based check needs to run everything a 2nd time, so it will be slower
>>>
>>> also ffprobe is only good for side data from the demuxer.
>>> my patch tests all cases including side data from the encoder or any other
>>> source that gets forwarded to the muxer in each testcase.
>>
>> We could extend showinfo_bsf to print side data information.
> 
> Well, you argued a moment ago that its too much code (in framecrcenc)
> its not going to be less code if the same or more detailed information
> is printed in a showinfo_bsf
> 
> again, my suggestion is that this code should go to where side data is
> and then showinfo_bsf, framecrcenc and ffprobe can use it

I mean, showinfo_bsf could be adapted in a way ffprobe can invoke/parse, 
so all the related ffprobe code can be moved there.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".