Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
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 --- 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@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".
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
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 > > > > > > > --- > > > > > > > > > > > > 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 thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
On Mon, May 27, 2024 at 04:32:43PM -0300, James Almer wrote: > 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 > > > > > > --- > > > > > > > > > > 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? side data specific code doesnt belong in ffprobe what should instead be done is to have side data parsing code where the side data struct is Side data can be represented in at least 4 ways 1. one or more C structs 2. a serialized bytestream 3. json data 4. a human readable key value printout We have some of this in ffprobe, it should be closer to iamf.c/h and then used by framecrcenc (whatever format framecrcenc uses is not important) [...] thx -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
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 --- 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. 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-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".
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
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
---
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-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".
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
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 > > > > > --- > > > > > > > > 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. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Never trust a computer, one day, it may think you are the virus. -- Compn signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
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 > > > > --- > > > > > > 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. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is a danger to trust the dream we wish for rather than the science we have, -- Dr. Kenneth Brown signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
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 --- 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, so it does feel like it's not the right thing to do. ffprobe (and f_sidedata) are what should be used for actual integrity checks. And i insist that disabling printing the size (or any information about side data) from framecrc is needed for FATE tests, as we can't even use it to hash actual packet data otherwise. ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
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 > > --- > > 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) If you block this solution without providing another solution then i will bring this to the technical committee, to seek its guidance on the way forward. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Does the universe only have a finite lifespan? No, its going to go on forever, its just that you wont like living in it. -- Hiranya Peiri signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
On 5/27/2024 5:15 AM, 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 --- I am against this patch. Checksumming side data is a fundamentally wrong thing to do. framehash should be updated to also not print hashes, then. Problem is that it's versioned, so we would need to "deprecate" the current versions. There's also the issue of printing sizes, which for some structs (iamf, video_enc_params) it can vary depending on target, so my suggestion was adding an option to disable printing certain values like it. What do you think? ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
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 > --- I am against this patch. Checksumming side data is a fundamentally wrong thing to do. -- Anton Khirnov ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
On 4/30/2024 9:40 PM, Michael Niedermayer wrote:
On Tue, Apr 30, 2024 at 08:29:07PM -0300, James Almer wrote:
On 4/30/2024 8:25 PM, Michael Niedermayer wrote:
On Sun, Apr 28, 2024 at 12:43:50AM -0300, James Almer wrote:
On 4/27/2024 9:07 AM, Michael Niedermayer wrote:
On Sat, Apr 27, 2024 at 12:44:18PM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
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
---
libavformat/framecrcenc.c | 76 +-
tests/ref/fate/autorotate |2 +-
tests/ref/fate/cover-art-mp3-id3v2-remux|2 +-
tests/ref/fate/ffmpeg-bsf-input | 10 +-
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/iamf-5_1-copy| 208 ++--
tests/ref/fate/iamf-5_1-demux | 208 ++--
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 +-
21 files changed, 1513 insertions(+), 1443 deletions(-)
diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcenc.c
index ce306a6c498..e71bfbd8777 100644
--- a/libavformat/framecrcenc.c
+++ b/libavformat/framecrcenc.c
@@ -21,8 +21,10 @@
#include
+#include "config.h"
#include "libavutil/adler32.h"
#include "libavutil/avstring.h"
+#include "libavutil/intreadwrite.h"
#include "libavcodec/codec_id.h"
#include "libavcodec/codec_par.h"
@@ -48,6 +50,17 @@ 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));
+}
+}
+
static int framecrc_write_packet(struct AVFormatContext *s, AVPacket *pkt)
{
uint32_t crc = av_adler32_update(0, pkt->data, pkt->size);
@@ -58,11 +71,68 @@ 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;
This change is wrong.
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; iside_data_elems; i++) {
+const AVPacketSideData *const sd = >side_data[i];
+const uint8_t *data = sd->data;
+uint32_t side_data_crc = 0;
+
+switch (sd->type) {
+#if HAVE_BIGENDIAN
+uint8_t bswap_buf[FFMAX(sizeof(AVCPBProperties),
+sizeof(AVProducerReferenceTime))];
+case AV_PKT_DATA_PALETTE:
+case AV_PKT_DATA_REPLAYGAIN:
+case AV_PKT_DATA_DISPLAYMATRIX:
+case AV_PKT_DATA_STEREO3D:
+case AV_PKT_DATA_AUDIO_SERVICE_TYPE:
+case AV_PKT_DATA_FALLBACK_TRACK:
+case AV_PKT_DATA_MASTERING_DISPLAY_METADATA:
+case AV_PKT_DATA_SPHERICAL:
+case AV_PKT_DATA_CONTENT_LIGHT_LEVEL:
+case AV_PKT_DATA_S12M_TIMECODE:
+for (size_t j = 0; j < sd->size / 4; j++) {
+uint8_t buf[4];
+AV_WL32(buf, AV_RB32(sd->data + 4 * j));
+side_data_crc = av_adler32_update(side_data_crc, buf, 4);
+}
+break;
+case AV_PKT_DATA_CPB_PROPERTIES:
+#define BSWAP(struct, field) bswap(bswap_buf, offsetof(struct, field),
sizeof(((struct){0}).field))
+if (sd->size == sizeof(AVCPBProperties)) {
+memcpy(bswap_buf, sd->data, sizeof(AVCPBProperties));
+data = bswap_buf;
+BSWAP(AVCPBProperties, max_bitrate);
+BSWAP(AVCPBProperties, min_bitrate);
+
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
On Tue, Apr 30, 2024 at 08:29:07PM -0300, James Almer wrote:
> On 4/30/2024 8:25 PM, Michael Niedermayer wrote:
> > On Sun, Apr 28, 2024 at 12:43:50AM -0300, James Almer wrote:
> > > On 4/27/2024 9:07 AM, Michael Niedermayer wrote:
> > > > On Sat, Apr 27, 2024 at 12:44:18PM +0200, Andreas Rheinhardt wrote:
> > > > > Michael Niedermayer:
> > > > > > 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
> > > > > > ---
> > > > > >libavformat/framecrcenc.c | 76 +-
> > > > > >tests/ref/fate/autorotate |2 +-
> > > > > >tests/ref/fate/cover-art-mp3-id3v2-remux|2 +-
> > > > > >tests/ref/fate/ffmpeg-bsf-input | 10 +-
> > > > > >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/iamf-5_1-copy| 208 ++--
> > > > > >tests/ref/fate/iamf-5_1-demux | 208 ++--
> > > > > >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 +-
> > > > > >21 files changed, 1513 insertions(+), 1443 deletions(-)
> > > > > >
> > > > > > diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcenc.c
> > > > > > index ce306a6c498..e71bfbd8777 100644
> > > > > > --- a/libavformat/framecrcenc.c
> > > > > > +++ b/libavformat/framecrcenc.c
> > > > > > @@ -21,8 +21,10 @@
> > > > > >#include
> > > > > > +#include "config.h"
> > > > > >#include "libavutil/adler32.h"
> > > > > >#include "libavutil/avstring.h"
> > > > > > +#include "libavutil/intreadwrite.h"
> > > > > >#include "libavcodec/codec_id.h"
> > > > > >#include "libavcodec/codec_par.h"
> > > > > > @@ -48,6 +50,17 @@ 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));
> > > > > > +}
> > > > > > +}
> > > > > > +
> > > > > >static int framecrc_write_packet(struct AVFormatContext *s,
> > > > > > AVPacket *pkt)
> > > > > >{
> > > > > >uint32_t crc = av_adler32_update(0, pkt->data, pkt->size);
> > > > > > @@ -58,11 +71,68 @@ 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;
> > > > >
> > > > > This change is wrong.
> > > > >
> > > > > >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; iside_data_elems; i++) {
> > > > > > +const AVPacketSideData *const sd = >side_data[i];
> > > > > > +const uint8_t *data = sd->data;
> > > > > > +uint32_t side_data_crc = 0;
> > > > > > +
> > > > > > +switch (sd->type) {
> > > > > > +#if HAVE_BIGENDIAN
> > > > > > +uint8_t bswap_buf[FFMAX(sizeof(AVCPBProperties),
> > > > > > +
> > > > > > sizeof(AVProducerReferenceTime))];
> > > > > > +case AV_PKT_DATA_PALETTE:
> > > > > > +case AV_PKT_DATA_REPLAYGAIN:
> > > > > > +case AV_PKT_DATA_DISPLAYMATRIX:
> > > > > > +case AV_PKT_DATA_STEREO3D:
> > > > > > +case
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
On 4/30/2024 8:25 PM, Michael Niedermayer wrote:
On Sun, Apr 28, 2024 at 12:43:50AM -0300, James Almer wrote:
On 4/27/2024 9:07 AM, Michael Niedermayer wrote:
On Sat, Apr 27, 2024 at 12:44:18PM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
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
---
libavformat/framecrcenc.c | 76 +-
tests/ref/fate/autorotate |2 +-
tests/ref/fate/cover-art-mp3-id3v2-remux|2 +-
tests/ref/fate/ffmpeg-bsf-input | 10 +-
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/iamf-5_1-copy| 208 ++--
tests/ref/fate/iamf-5_1-demux | 208 ++--
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 +-
21 files changed, 1513 insertions(+), 1443 deletions(-)
diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcenc.c
index ce306a6c498..e71bfbd8777 100644
--- a/libavformat/framecrcenc.c
+++ b/libavformat/framecrcenc.c
@@ -21,8 +21,10 @@
#include
+#include "config.h"
#include "libavutil/adler32.h"
#include "libavutil/avstring.h"
+#include "libavutil/intreadwrite.h"
#include "libavcodec/codec_id.h"
#include "libavcodec/codec_par.h"
@@ -48,6 +50,17 @@ 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));
+}
+}
+
static int framecrc_write_packet(struct AVFormatContext *s, AVPacket *pkt)
{
uint32_t crc = av_adler32_update(0, pkt->data, pkt->size);
@@ -58,11 +71,68 @@ 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;
This change is wrong.
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; iside_data_elems; i++) {
+const AVPacketSideData *const sd = >side_data[i];
+const uint8_t *data = sd->data;
+uint32_t side_data_crc = 0;
+
+switch (sd->type) {
+#if HAVE_BIGENDIAN
+uint8_t bswap_buf[FFMAX(sizeof(AVCPBProperties),
+sizeof(AVProducerReferenceTime))];
+case AV_PKT_DATA_PALETTE:
+case AV_PKT_DATA_REPLAYGAIN:
+case AV_PKT_DATA_DISPLAYMATRIX:
+case AV_PKT_DATA_STEREO3D:
+case AV_PKT_DATA_AUDIO_SERVICE_TYPE:
+case AV_PKT_DATA_FALLBACK_TRACK:
+case AV_PKT_DATA_MASTERING_DISPLAY_METADATA:
+case AV_PKT_DATA_SPHERICAL:
+case AV_PKT_DATA_CONTENT_LIGHT_LEVEL:
+case AV_PKT_DATA_S12M_TIMECODE:
+for (size_t j = 0; j < sd->size / 4; j++) {
+uint8_t buf[4];
+AV_WL32(buf, AV_RB32(sd->data + 4 * j));
+side_data_crc = av_adler32_update(side_data_crc, buf, 4);
+}
+break;
+case AV_PKT_DATA_CPB_PROPERTIES:
+#define BSWAP(struct, field) bswap(bswap_buf, offsetof(struct, field),
sizeof(((struct){0}).field))
+if (sd->size == sizeof(AVCPBProperties)) {
+memcpy(bswap_buf, sd->data, sizeof(AVCPBProperties));
+data = bswap_buf;
+BSWAP(AVCPBProperties, max_bitrate);
+BSWAP(AVCPBProperties, min_bitrate);
+BSWAP(AVCPBProperties, avg_bitrate);
+BSWAP(AVCPBProperties, buffer_size);
+BSWAP(AVCPBProperties, vbv_delay);
+
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
On Sun, Apr 28, 2024 at 12:43:50AM -0300, James Almer wrote:
> On 4/27/2024 9:07 AM, Michael Niedermayer wrote:
> > On Sat, Apr 27, 2024 at 12:44:18PM +0200, Andreas Rheinhardt wrote:
> > > Michael Niedermayer:
> > > > 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
> > > > ---
> > > > libavformat/framecrcenc.c | 76 +-
> > > > tests/ref/fate/autorotate |2 +-
> > > > tests/ref/fate/cover-art-mp3-id3v2-remux|2 +-
> > > > tests/ref/fate/ffmpeg-bsf-input | 10 +-
> > > > 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/iamf-5_1-copy| 208 ++--
> > > > tests/ref/fate/iamf-5_1-demux | 208 ++--
> > > > 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 +-
> > > > 21 files changed, 1513 insertions(+), 1443 deletions(-)
> > > >
> > > > diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcenc.c
> > > > index ce306a6c498..e71bfbd8777 100644
> > > > --- a/libavformat/framecrcenc.c
> > > > +++ b/libavformat/framecrcenc.c
> > > > @@ -21,8 +21,10 @@
> > > > #include
> > > > +#include "config.h"
> > > > #include "libavutil/adler32.h"
> > > > #include "libavutil/avstring.h"
> > > > +#include "libavutil/intreadwrite.h"
> > > > #include "libavcodec/codec_id.h"
> > > > #include "libavcodec/codec_par.h"
> > > > @@ -48,6 +50,17 @@ 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));
> > > > +}
> > > > +}
> > > > +
> > > > static int framecrc_write_packet(struct AVFormatContext *s, AVPacket
> > > > *pkt)
> > > > {
> > > > uint32_t crc = av_adler32_update(0, pkt->data, pkt->size);
> > > > @@ -58,11 +71,68 @@ 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;
> > >
> > > This change is wrong.
> > >
> > > > 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; iside_data_elems; i++) {
> > > > +const AVPacketSideData *const sd = >side_data[i];
> > > > +const uint8_t *data = sd->data;
> > > > +uint32_t side_data_crc = 0;
> > > > +
> > > > +switch (sd->type) {
> > > > +#if HAVE_BIGENDIAN
> > > > +uint8_t bswap_buf[FFMAX(sizeof(AVCPBProperties),
> > > > +
> > > > sizeof(AVProducerReferenceTime))];
> > > > +case AV_PKT_DATA_PALETTE:
> > > > +case AV_PKT_DATA_REPLAYGAIN:
> > > > +case AV_PKT_DATA_DISPLAYMATRIX:
> > > > +case AV_PKT_DATA_STEREO3D:
> > > > +case AV_PKT_DATA_AUDIO_SERVICE_TYPE:
> > > > +case AV_PKT_DATA_FALLBACK_TRACK:
> > > > +case AV_PKT_DATA_MASTERING_DISPLAY_METADATA:
> > > > +case AV_PKT_DATA_SPHERICAL:
> > > > +case AV_PKT_DATA_CONTENT_LIGHT_LEVEL:
> > > > +case AV_PKT_DATA_S12M_TIMECODE:
> > > > +for (size_t j = 0; j < sd->size / 4; j++) {
> > > > +uint8_t buf[4];
> > > > +AV_WL32(buf, AV_RB32(sd->data + 4 * j));
> > > > +side_data_crc =
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
On 4/27/2024 9:07 AM, Michael Niedermayer wrote:
On Sat, Apr 27, 2024 at 12:44:18PM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
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
---
libavformat/framecrcenc.c | 76 +-
tests/ref/fate/autorotate |2 +-
tests/ref/fate/cover-art-mp3-id3v2-remux|2 +-
tests/ref/fate/ffmpeg-bsf-input | 10 +-
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/iamf-5_1-copy| 208 ++--
tests/ref/fate/iamf-5_1-demux | 208 ++--
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 +-
21 files changed, 1513 insertions(+), 1443 deletions(-)
diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcenc.c
index ce306a6c498..e71bfbd8777 100644
--- a/libavformat/framecrcenc.c
+++ b/libavformat/framecrcenc.c
@@ -21,8 +21,10 @@
#include
+#include "config.h"
#include "libavutil/adler32.h"
#include "libavutil/avstring.h"
+#include "libavutil/intreadwrite.h"
#include "libavcodec/codec_id.h"
#include "libavcodec/codec_par.h"
@@ -48,6 +50,17 @@ 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));
+}
+}
+
static int framecrc_write_packet(struct AVFormatContext *s, AVPacket *pkt)
{
uint32_t crc = av_adler32_update(0, pkt->data, pkt->size);
@@ -58,11 +71,68 @@ 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;
This change is wrong.
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; iside_data_elems; i++) {
+const AVPacketSideData *const sd = >side_data[i];
+const uint8_t *data = sd->data;
+uint32_t side_data_crc = 0;
+
+switch (sd->type) {
+#if HAVE_BIGENDIAN
+uint8_t bswap_buf[FFMAX(sizeof(AVCPBProperties),
+sizeof(AVProducerReferenceTime))];
+case AV_PKT_DATA_PALETTE:
+case AV_PKT_DATA_REPLAYGAIN:
+case AV_PKT_DATA_DISPLAYMATRIX:
+case AV_PKT_DATA_STEREO3D:
+case AV_PKT_DATA_AUDIO_SERVICE_TYPE:
+case AV_PKT_DATA_FALLBACK_TRACK:
+case AV_PKT_DATA_MASTERING_DISPLAY_METADATA:
+case AV_PKT_DATA_SPHERICAL:
+case AV_PKT_DATA_CONTENT_LIGHT_LEVEL:
+case AV_PKT_DATA_S12M_TIMECODE:
+for (size_t j = 0; j < sd->size / 4; j++) {
+uint8_t buf[4];
+AV_WL32(buf, AV_RB32(sd->data + 4 * j));
+side_data_crc = av_adler32_update(side_data_crc, buf, 4);
+}
+break;
+case AV_PKT_DATA_CPB_PROPERTIES:
+#define BSWAP(struct, field) bswap(bswap_buf, offsetof(struct, field),
sizeof(((struct){0}).field))
+if (sd->size == sizeof(AVCPBProperties)) {
+memcpy(bswap_buf, sd->data, sizeof(AVCPBProperties));
+data = bswap_buf;
+BSWAP(AVCPBProperties, max_bitrate);
+BSWAP(AVCPBProperties, min_bitrate);
+BSWAP(AVCPBProperties, avg_bitrate);
+BSWAP(AVCPBProperties, buffer_size);
+BSWAP(AVCPBProperties, vbv_delay);
+}
+goto pod;
+case AV_PKT_DATA_PRFT:
+if (sd->size ==
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
On Sat, Apr 27, 2024 at 12:44:18PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > 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
> > ---
> > libavformat/framecrcenc.c | 76 +-
> > tests/ref/fate/autorotate |2 +-
> > tests/ref/fate/cover-art-mp3-id3v2-remux|2 +-
> > tests/ref/fate/ffmpeg-bsf-input | 10 +-
> > 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/iamf-5_1-copy| 208 ++--
> > tests/ref/fate/iamf-5_1-demux | 208 ++--
> > 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 +-
> > 21 files changed, 1513 insertions(+), 1443 deletions(-)
> >
> > diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcenc.c
> > index ce306a6c498..e71bfbd8777 100644
> > --- a/libavformat/framecrcenc.c
> > +++ b/libavformat/framecrcenc.c
> > @@ -21,8 +21,10 @@
> >
> > #include
> >
> > +#include "config.h"
> > #include "libavutil/adler32.h"
> > #include "libavutil/avstring.h"
> > +#include "libavutil/intreadwrite.h"
> >
> > #include "libavcodec/codec_id.h"
> > #include "libavcodec/codec_par.h"
> > @@ -48,6 +50,17 @@ 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));
> > +}
> > +}
> > +
> > static int framecrc_write_packet(struct AVFormatContext *s, AVPacket *pkt)
> > {
> > uint32_t crc = av_adler32_update(0, pkt->data, pkt->size);
> > @@ -58,11 +71,68 @@ 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;
>
> This change is wrong.
>
> > 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; iside_data_elems; i++) {
> > +const AVPacketSideData *const sd = >side_data[i];
> > +const uint8_t *data = sd->data;
> > +uint32_t side_data_crc = 0;
> > +
> > +switch (sd->type) {
> > +#if HAVE_BIGENDIAN
> > +uint8_t bswap_buf[FFMAX(sizeof(AVCPBProperties),
> > +sizeof(AVProducerReferenceTime))];
> > +case AV_PKT_DATA_PALETTE:
> > +case AV_PKT_DATA_REPLAYGAIN:
> > +case AV_PKT_DATA_DISPLAYMATRIX:
> > +case AV_PKT_DATA_STEREO3D:
> > +case AV_PKT_DATA_AUDIO_SERVICE_TYPE:
> > +case AV_PKT_DATA_FALLBACK_TRACK:
> > +case AV_PKT_DATA_MASTERING_DISPLAY_METADATA:
> > +case AV_PKT_DATA_SPHERICAL:
> > +case AV_PKT_DATA_CONTENT_LIGHT_LEVEL:
> > +case AV_PKT_DATA_S12M_TIMECODE:
> > +for (size_t j = 0; j < sd->size / 4; j++) {
> > +uint8_t buf[4];
> > +AV_WL32(buf, AV_RB32(sd->data + 4 * j));
> > +side_data_crc = av_adler32_update(side_data_crc, buf,
> > 4);
> > +}
> > +break;
> > +case AV_PKT_DATA_CPB_PROPERTIES:
> > +#define BSWAP(struct, field) bswap(bswap_buf, offsetof(struct, field),
> > sizeof(((struct){0}).field))
> > +if (sd->size == sizeof(AVCPBProperties)) {
> > +memcpy(bswap_buf, sd->data, sizeof(AVCPBProperties));
> > +data = bswap_buf;
> > +BSWAP(AVCPBProperties, max_bitrate);
> > +
Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
Michael Niedermayer:
> 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
> ---
> libavformat/framecrcenc.c | 76 +-
> tests/ref/fate/autorotate |2 +-
> tests/ref/fate/cover-art-mp3-id3v2-remux|2 +-
> tests/ref/fate/ffmpeg-bsf-input | 10 +-
> 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/iamf-5_1-copy| 208 ++--
> tests/ref/fate/iamf-5_1-demux | 208 ++--
> 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 +-
> 21 files changed, 1513 insertions(+), 1443 deletions(-)
>
> diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcenc.c
> index ce306a6c498..e71bfbd8777 100644
> --- a/libavformat/framecrcenc.c
> +++ b/libavformat/framecrcenc.c
> @@ -21,8 +21,10 @@
>
> #include
>
> +#include "config.h"
> #include "libavutil/adler32.h"
> #include "libavutil/avstring.h"
> +#include "libavutil/intreadwrite.h"
>
> #include "libavcodec/codec_id.h"
> #include "libavcodec/codec_par.h"
> @@ -48,6 +50,17 @@ 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));
> +}
> +}
> +
> static int framecrc_write_packet(struct AVFormatContext *s, AVPacket *pkt)
> {
> uint32_t crc = av_adler32_update(0, pkt->data, pkt->size);
> @@ -58,11 +71,68 @@ 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;
This change is wrong.
> 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; iside_data_elems; i++) {
> +const AVPacketSideData *const sd = >side_data[i];
> +const uint8_t *data = sd->data;
> +uint32_t side_data_crc = 0;
> +
> +switch (sd->type) {
> +#if HAVE_BIGENDIAN
> +uint8_t bswap_buf[FFMAX(sizeof(AVCPBProperties),
> +sizeof(AVProducerReferenceTime))];
> +case AV_PKT_DATA_PALETTE:
> +case AV_PKT_DATA_REPLAYGAIN:
> +case AV_PKT_DATA_DISPLAYMATRIX:
> +case AV_PKT_DATA_STEREO3D:
> +case AV_PKT_DATA_AUDIO_SERVICE_TYPE:
> +case AV_PKT_DATA_FALLBACK_TRACK:
> +case AV_PKT_DATA_MASTERING_DISPLAY_METADATA:
> +case AV_PKT_DATA_SPHERICAL:
> +case AV_PKT_DATA_CONTENT_LIGHT_LEVEL:
> +case AV_PKT_DATA_S12M_TIMECODE:
> +for (size_t j = 0; j < sd->size / 4; j++) {
> +uint8_t buf[4];
> +AV_WL32(buf, AV_RB32(sd->data + 4 * j));
> +side_data_crc = av_adler32_update(side_data_crc, buf, 4);
> +}
> +break;
> +case AV_PKT_DATA_CPB_PROPERTIES:
> +#define BSWAP(struct, field) bswap(bswap_buf, offsetof(struct, field),
> sizeof(((struct){0}).field))
> +if (sd->size == sizeof(AVCPBProperties)) {
> +memcpy(bswap_buf, sd->data, sizeof(AVCPBProperties));
> +data = bswap_buf;
> +BSWAP(AVCPBProperties, max_bitrate);
> +BSWAP(AVCPBProperties, min_bitrate);
> +BSWAP(AVCPBProperties, avg_bitrate);
> +BSWAP(AVCPBProperties, buffer_size);
> +BSWAP(AVCPBProperties, vbv_delay);
> +}
> +goto pod;
> +case AV_PKT_DATA_PRFT:
> +
