[FFmpeg-devel] [PATCH 1/2] avformat/framehash: don't share write_header code with framecrc muxers

[James Almer]

On 4/12/2016 11:31 PM, Michael Niedermayer wrote:
> On Tue, Apr 12, 2016 at 07:32:44PM -0300, James Almer wrote:
>> The uncodedframecrc muxer didn't like the new ff_framehash_write_header signature.
>> This may also simplify further improvements to the output of framehash.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>  libavformat/Makefile                    |  8 ++++----
>>  libavformat/{framehash.c => framecrc.c} | 19 ++-----------------
>>  libavformat/framecrcenc.c               |  2 +-
>>  libavformat/hashenc.c                   | 27 +++++++++++++++++++++++++--
>>  libavformat/internal.h                  |  2 +-
>>  libavformat/uncodedframecrcenc.c        |  2 +-
>>  6 files changed, 34 insertions(+), 26 deletions(-)
>>  rename libavformat/{framehash.c => framecrc.c} (54%)
> 
> Iam not sure the version code is usefull, what is its usecase ?

Parsing scripts or something that expects the output of "version 1" to be
the same as it has been for years now, maybe? I'm not the one that added
a version line and the corresponding AVOption to the muxers, so not sure
what was the original idea. I faintly recall a trac ticket about it.

> (if it has no use maybe it could just be removed and make your life
>  easier ?)

If that's preferred then sure.

> 
> If you want to keep the version 1vs2 code maybe all the hash muxers
> could use teh same context, this would also possibly allow more
> future code sharing

The output of framecrc would change considerably if we make it an alias
of framehash, like framemd5 currently is. That's why I thought keeping
it separate was a better idea. But i agree having everything be framehash
would simplify the code considerably.

> 
> either way, IMO the latest version should be default (its compatible
> just has more fields)

I'll make it default once I'm done changing the output, so the fate ref
files are changed only once, or at least as few times as possible. I still
haven't added sidedata checksum for example.

> we never would want to use a old version for checking
> anything as it simply checks less at no advantage
> also its IMO important that the extra fields are enabled for the
> actual fate tests, this patchset would (please corect me if iam wrong)
> remove this support from the actually used (in fate) framecrc

True. I wrote this with the intention of keeping framecrc intact, which
is pretty much the opposite of your intention to add the extra output to it.

In that case guess I'll just turn framecrc into an alias of framehash with
adler32 as default. The commit updating ref files will be massive, though,
because AVHash and current framecrc initialize adler32 with different
values.

> 
> and for regression testing a fast checksum should be used, fate doesnt
> need or benefit from a crypto hash like md5 or sha256, it just makes
> it slower.
> a crap hash like adler32 actually makes interpreting hash differences
> possible as off by 1 differences result in small differences in the
> adler32 checksum while a sha256 differecnce would not allow seperating
> a off by one input difference from a totally different input
> 
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>