[FFmpeg-devel,PATCHv2,3/4] avcodec/svq1enc: output ident string in extradata field

This will enable the acurate identification of FFmpeg produced
SVQ1 streams, should there be new bugs found in the encoder.
---
 libavcodec/svq1enc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

On 10/19/2022 10:30 PM, Peter Ross wrote:
> This will enable the acurate identification of FFmpeg produced
> SVQ1 streams, should there be new bugs found in the encoder.
> ---
>   libavcodec/svq1enc.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/libavcodec/svq1enc.c b/libavcodec/svq1enc.c
> index 9bd5a04368..6aacaef88d 100644
> --- a/libavcodec/svq1enc.c
> +++ b/libavcodec/svq1enc.c
> @@ -41,6 +41,7 @@
>   #include "svq1.h"
>   #include "svq1encdsp.h"
>   #include "svq1enc_cb.h"
> +#include "version.h"
>   
>   #include "libavutil/avassert.h"
>   #include "libavutil/frame.h"
> @@ -628,6 +629,14 @@ static av_cold int svq1_encode_init(AVCodecContext *avctx)
>   
>       ff_h263_encode_init(&s->m); // mv_penalty
>   
> +    if (!(s->avctx->flags & AV_CODEC_FLAG_BITEXACT)) {
> +        avctx->extradata = av_malloc(sizeof(LIBAVCODEC_IDENT));
> +        if (!avctx->extradata)
> +            return AVERROR(ENOMEM);
> +        memcpy(avctx->extradata, LIBAVCODEC_IDENT, sizeof(LIBAVCODEC_IDENT));
> +        avctx->extradata_size = sizeof(LIBAVCODEC_IDENT);

Can you explain what effect is this meant to have and where? Do or 
should muxers like mp4 and matroska expect extradata from svq1 streams 
that they are meant to write to the output file?

> +    }
> +
>       return 0;
>   }
>   
> 
> 
> _______________________________________________
> 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".

On Wed, Oct 19, 2022 at 10:42:39PM -0300, James Almer wrote:
> On 10/19/2022 10:30 PM, Peter Ross wrote:
> > This will enable the acurate identification of FFmpeg produced
> > SVQ1 streams, should there be new bugs found in the encoder.
> > ---
> >   libavcodec/svq1enc.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> > 
> > diff --git a/libavcodec/svq1enc.c b/libavcodec/svq1enc.c
> > index 9bd5a04368..6aacaef88d 100644
> > --- a/libavcodec/svq1enc.c
> > +++ b/libavcodec/svq1enc.c
> > @@ -41,6 +41,7 @@
> >   #include "svq1.h"
> >   #include "svq1encdsp.h"
> >   #include "svq1enc_cb.h"
> > +#include "version.h"
> >   #include "libavutil/avassert.h"
> >   #include "libavutil/frame.h"
> > @@ -628,6 +629,14 @@ static av_cold int svq1_encode_init(AVCodecContext *avctx)
> >       ff_h263_encode_init(&s->m); // mv_penalty
> > +    if (!(s->avctx->flags & AV_CODEC_FLAG_BITEXACT)) {
> > +        avctx->extradata = av_malloc(sizeof(LIBAVCODEC_IDENT));
> > +        if (!avctx->extradata)
> > +            return AVERROR(ENOMEM);
> > +        memcpy(avctx->extradata, LIBAVCODEC_IDENT, sizeof(LIBAVCODEC_IDENT));
> > +        avctx->extradata_size = sizeof(LIBAVCODEC_IDENT);
> 
> Can you explain what effect is this meant to have and where? Do or should
> muxers like mp4 and matroska expect extradata from svq1 streams that they
> are meant to write to the output file?

the intention is to populate MOV stsd atom with a unique version number, such
that if another encoder bug is discovered, we can acurately detect the version
of the encoder and apply workarounds. the mov stsd atom is mapped to avctx->extradata.

we already use this approach for other encoders. the MPEG4 ASP video encoder
inserts the codec ident in a free text part of the header. there is no similar
free text space in the SVQ1 bitstream. fortunately the official/binary decoder
ignores the stsd atom, so i propose to use that. the SVQ1 official decoder has
not received updates for a long time and i dont expect it to change. the windows
binaries are marked end of life, and have not recieved update since 2009.

for other file formats, i expect them just to carry the extradata if they can.
this already works fine for avi. in the case of matroska, FFmpeg already inserts
the extradata in a V_QUICKTIME block per the specification
(https://www.matroska.org/technical/codec_specs.html ), so nothing to do there.

however now that i look closely, FFmpeg also adds a custom atom around the extradata
(commit in 8456bd2c0f3b08756f353646fe3b40a6772e665e), and doesn't strip this away
on demux. this is wrong, imho. 

it may be worth improving this patch set the following two ways:

 1) modify the SVQ1 encoder ident extradata to look like an stsd atom, by
     inserting length and tag fields at the beginning.

 2) modify the matroska muxer *not* to emit this custom atom for codecs who's
    extradta is also in atom format (SVQ1 and SVQ3).

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)

On Thu, Oct 20, 2022 at 12:30:33PM +1100, Peter Ross wrote:
> This will enable the acurate identification of FFmpeg produced
> SVQ1 streams, should there be new bugs found in the encoder.
> ---
>  libavcodec/svq1enc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/libavcodec/svq1enc.c b/libavcodec/svq1enc.c
> index 9bd5a04368..6aacaef88d 100644
> --- a/libavcodec/svq1enc.c
> +++ b/libavcodec/svq1enc.c
> @@ -41,6 +41,7 @@
>  #include "svq1.h"
>  #include "svq1encdsp.h"
>  #include "svq1enc_cb.h"
> +#include "version.h"
>  
>  #include "libavutil/avassert.h"
>  #include "libavutil/frame.h"
> @@ -628,6 +629,14 @@ static av_cold int svq1_encode_init(AVCodecContext *avctx)
>  
>      ff_h263_encode_init(&s->m); // mv_penalty
>  
> +    if (!(s->avctx->flags & AV_CODEC_FLAG_BITEXACT)) {
> +        avctx->extradata = av_malloc(sizeof(LIBAVCODEC_IDENT));
> +        if (!avctx->extradata)
> +            return AVERROR(ENOMEM);
> +        memcpy(avctx->extradata, LIBAVCODEC_IDENT, sizeof(LIBAVCODEC_IDENT));
> +        avctx->extradata_size = sizeof(LIBAVCODEC_IDENT);

This could still store the "Lavc" without the version #
I thought we did that more consistently but it seems only some
encoders do it like aac:
libavcodec/aacenc.c-    const int bitexact = avctx->flags & AV_CODEC_FLAG_BITEXACT;
libavcodec/aacenc.c:    const char *aux_data = bitexact ? "Lavc" : LIBAVCODEC_IDENT;

thx

[...]