[FFmpeg-devel] [PATCH 12/12] lavf/framecrcenc: do not hash side data

[Michael Niedermayer]

On Fri, May 14, 2021 at 09:42:10AM -0300, James Almer wrote:
> On 5/14/2021 5:01 AM, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2021-05-10 16:06:01)
> > > On Sun, Apr 25, 2021 at 09:03:20AM +0200, Anton Khirnov wrote:
> > > > There are no guarantees that all side data types have the same
> > > > representation on all platforms.
> > > 
> > > > @@ -65,63 +51,6 @@ static int framecrc_write_packet(struct AVFormatContext *s, AVPacket *pkt)
> > > >                pkt->stream_index, pkt->dts, pkt->pts, pkt->duration, pkt->size, crc);
> > > >       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);
> > > 
> > > The number and type of the side data elements should not be affected
> > > by the problem described.
> > > Maybe we could leave that. Bugs could manifest as the absence or addition
> > > of side data, seeing that in framecrc_write_packet() output may be usefull
> > 
> > No strong opinion here - patches welcome I guess.
> 
> I can do it, but it will be a "breaking" change, assuming there are parsers
> that read the output of this muxer.

does anyone know of such parsers ?
or does anyone have an idea what such parser could be used for ?


> Right now you removed all the trailing properties, which while also
> breaking, a sane parser made for the old output can simply assume that the
> line was truncated. But if we re-add the side data amount and sizes for each
> of them without the hashes, things can get parsed the wrong way.
> 
> Namely, it used to be:
> > 0,          0,          0,       16,    57008, 0x43416399, S=2,        8, 0x08e5014f,       88, 0xd65a04db
> 
> And now it will be something like:
> > 0,          0,          0,       16,    57008, 0x43416399, S=2,        8,       88
> 
> So the question is, do we care about this? The framehash/framemd5 muxer
> prints a version number, which lets us make breaking additions parsers can
> then properly handle. Should we then just consider that parsing framecrc
> output is not a supported scenario?

the version should probably be increased 
we also could leave the checksum in there but pick one that does not change
when 0 padding is added or endian changes, an example would be a sum of all
bytes. But in the strictest interpretation i guess that still would not be
platform independant.

Thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210514/d5f41a25/attachment.sig>