diff mbox series

[FFmpeg-devel,v2,1/3] avcodec/snowenc: AV_CODEC_CAP_ENCODER_RECON_FRAME support

Message ID 20230319194240.15001-1-michael@niedermayer.cc
State Accepted
Commit 006136efaf440d1876b06aae8a6403730c09069f
Headers show
Series [FFmpeg-devel,v2,1/3] avcodec/snowenc: AV_CODEC_CAP_ENCODER_RECON_FRAME support | expand

Commit Message

Michael Niedermayer March 19, 2023, 7:42 p.m. UTC 
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/snowenc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

James Almer March 20, 2023, 10:07 p.m. UTC | #1 
On 3/19/2023 4:42 PM, Michael Niedermayer wrote:
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavcodec/snowenc.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
> index 658684c575..5fb5906ed8 100644
> --- a/libavcodec/snowenc.c
> +++ b/libavcodec/snowenc.c
> @@ -26,6 +26,7 @@
>   #include "avcodec.h"
>   #include "codec_internal.h"
>   #include "encode.h"
> +#include "internal.h" //For AVCodecInternal.recon_frame
>   #include "me_cmp.h"
>   #include "packet_internal.h"
>   #include "snow_dwt.h"
> @@ -1576,6 +1577,7 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>   {
>       SnowContext *s = avctx->priv_data;
>       RangeCoder * const c= &s->c;
> +    AVCodecInternal *avci = avctx->internal;
>       AVFrame *pic;
>       const int width= s->avctx->width;
>       const int height= s->avctx->height;
> @@ -1877,6 +1879,10 @@ redo_frame:
>                                      s->encoding_error,
>                                      (s->avctx->flags&AV_CODEC_FLAG_PSNR) ? SNOW_MAX_PLANES : 0,
>                                      s->current_picture->pict_type);
> +    if (s->avctx->flags & AV_CODEC_FLAG_RECON_FRAME) {
> +        av_frame_unref(avci->recon_frame);
> +        av_frame_ref(avci->recon_frame, s->current_picture);
> +    }
>   
>       pkt->size = ff_rac_terminate(c, 0);
>       if (s->current_picture->key_frame)
> @@ -1934,7 +1940,9 @@ const FFCodec ff_snow_encoder = {
>       CODEC_LONG_NAME("Snow"),
>       .p.type         = AVMEDIA_TYPE_VIDEO,
>       .p.id           = AV_CODEC_ID_SNOW,
> -    .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE,
> +    .p.capabilities = AV_CODEC_CAP_DR1 |
> +                      AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
> +                      AV_CODEC_CAP_ENCODER_RECON_FRAME,
>       .priv_data_size = sizeof(SnowContext),
>       .init           = encode_init,
>       FF_CODEC_ENCODE_CB(encode_frame),

Testing with the tool from 
https://git.khirnov.net/libav.git/commit/?h=recon&id=d78e17840347abfe10b2bc7e789c60665de56e1f 
(Which i hope will be upstreamed eventually) i get

$ tools/enc_recon_frame_test input.y4m snow flags=+bitexact 10
Checksum mismatch in frame ts=0, plane 0

I don't know if this really matters for snow (is it mean to be 
bitexact?), but it seems the very first frame returned by the 
reconstructed frame API is not the same as the one output by the snow 
decoder. The rest seem to match, though.
Michael Niedermayer March 23, 2023, 9:50 a.m. UTC | #2 
On Mon, Mar 20, 2023 at 07:07:59PM -0300, James Almer wrote:
> On 3/19/2023 4:42 PM, Michael Niedermayer wrote:
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavcodec/snowenc.c | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
> > index 658684c575..5fb5906ed8 100644
> > --- a/libavcodec/snowenc.c
> > +++ b/libavcodec/snowenc.c
> > @@ -26,6 +26,7 @@
> >   #include "avcodec.h"
> >   #include "codec_internal.h"
> >   #include "encode.h"
> > +#include "internal.h" //For AVCodecInternal.recon_frame
> >   #include "me_cmp.h"
> >   #include "packet_internal.h"
> >   #include "snow_dwt.h"
> > @@ -1576,6 +1577,7 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
> >   {
> >       SnowContext *s = avctx->priv_data;
> >       RangeCoder * const c= &s->c;
> > +    AVCodecInternal *avci = avctx->internal;
> >       AVFrame *pic;
> >       const int width= s->avctx->width;
> >       const int height= s->avctx->height;
> > @@ -1877,6 +1879,10 @@ redo_frame:
> >                                      s->encoding_error,
> >                                      (s->avctx->flags&AV_CODEC_FLAG_PSNR) ? SNOW_MAX_PLANES : 0,
> >                                      s->current_picture->pict_type);
> > +    if (s->avctx->flags & AV_CODEC_FLAG_RECON_FRAME) {
> > +        av_frame_unref(avci->recon_frame);
> > +        av_frame_ref(avci->recon_frame, s->current_picture);
> > +    }
> >       pkt->size = ff_rac_terminate(c, 0);
> >       if (s->current_picture->key_frame)
> > @@ -1934,7 +1940,9 @@ const FFCodec ff_snow_encoder = {
> >       CODEC_LONG_NAME("Snow"),
> >       .p.type         = AVMEDIA_TYPE_VIDEO,
> >       .p.id           = AV_CODEC_ID_SNOW,
> > -    .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE,
> > +    .p.capabilities = AV_CODEC_CAP_DR1 |
> > +                      AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
> > +                      AV_CODEC_CAP_ENCODER_RECON_FRAME,
> >       .priv_data_size = sizeof(SnowContext),
> >       .init           = encode_init,
> >       FF_CODEC_ENCODE_CB(encode_frame),
> 
> Testing with the tool from https://git.khirnov.net/libav.git/commit/?h=recon&id=d78e17840347abfe10b2bc7e789c60665de56e1f
> (Which i hope will be upstreamed eventually) i get
> 
> $ tools/enc_recon_frame_test input.y4m snow flags=+bitexact 10
> Checksum mismatch in frame ts=0, plane 0

I cannot replicate this, using matrixbench and above command line i get no 
mismatches, where can i find the input.y4m you used ?

also would be nice if this tool would write a image showing the difference

thx

> 
> I don't know if this really matters for snow (is it mean to be bitexact?),
> but it seems the very first frame returned by the reconstructed frame API is
> not the same as the one output by the snow decoder. The rest seem to match,
> though.

This sounds rather strange, the first frame would be used as reference for
motion compensation so the subsequent frames matching after a mismatch is
"surprising"

thx

[...]
James Almer March 23, 2023, 11:32 a.m. UTC | #3 
On 3/23/2023 6:50 AM, Michael Niedermayer wrote:
> On Mon, Mar 20, 2023 at 07:07:59PM -0300, James Almer wrote:
>> On 3/19/2023 4:42 PM, Michael Niedermayer wrote:
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>    libavcodec/snowenc.c | 10 +++++++++-
>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
>>> index 658684c575..5fb5906ed8 100644
>>> --- a/libavcodec/snowenc.c
>>> +++ b/libavcodec/snowenc.c
>>> @@ -26,6 +26,7 @@
>>>    #include "avcodec.h"
>>>    #include "codec_internal.h"
>>>    #include "encode.h"
>>> +#include "internal.h" //For AVCodecInternal.recon_frame
>>>    #include "me_cmp.h"
>>>    #include "packet_internal.h"
>>>    #include "snow_dwt.h"
>>> @@ -1576,6 +1577,7 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>>>    {
>>>        SnowContext *s = avctx->priv_data;
>>>        RangeCoder * const c= &s->c;
>>> +    AVCodecInternal *avci = avctx->internal;
>>>        AVFrame *pic;
>>>        const int width= s->avctx->width;
>>>        const int height= s->avctx->height;
>>> @@ -1877,6 +1879,10 @@ redo_frame:
>>>                                       s->encoding_error,
>>>                                       (s->avctx->flags&AV_CODEC_FLAG_PSNR) ? SNOW_MAX_PLANES : 0,
>>>                                       s->current_picture->pict_type);
>>> +    if (s->avctx->flags & AV_CODEC_FLAG_RECON_FRAME) {
>>> +        av_frame_unref(avci->recon_frame);
>>> +        av_frame_ref(avci->recon_frame, s->current_picture);
>>> +    }
>>>        pkt->size = ff_rac_terminate(c, 0);
>>>        if (s->current_picture->key_frame)
>>> @@ -1934,7 +1940,9 @@ const FFCodec ff_snow_encoder = {
>>>        CODEC_LONG_NAME("Snow"),
>>>        .p.type         = AVMEDIA_TYPE_VIDEO,
>>>        .p.id           = AV_CODEC_ID_SNOW,
>>> -    .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE,
>>> +    .p.capabilities = AV_CODEC_CAP_DR1 |
>>> +                      AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
>>> +                      AV_CODEC_CAP_ENCODER_RECON_FRAME,
>>>        .priv_data_size = sizeof(SnowContext),
>>>        .init           = encode_init,
>>>        FF_CODEC_ENCODE_CB(encode_frame),
>>
>> Testing with the tool from https://git.khirnov.net/libav.git/commit/?h=recon&id=d78e17840347abfe10b2bc7e789c60665de56e1f
>> (Which i hope will be upstreamed eventually) i get
>>
>> $ tools/enc_recon_frame_test input.y4m snow flags=+bitexact 10
>> Checksum mismatch in frame ts=0, plane 0
> 
> I cannot replicate this, using matrixbench and above command line i get no
> mismatches, where can i find the input.y4m you used ?

It was https://0x0.st/sfRh.y4m

> 
> also would be nice if this tool would write a image showing the difference
> 
> thx
> 
>>
>> I don't know if this really matters for snow (is it mean to be bitexact?),
>> but it seems the very first frame returned by the reconstructed frame API is
>> not the same as the one output by the snow decoder. The rest seem to match,
>> though.
> 
> This sounds rather strange, the first frame would be used as reference for
> motion compensation so the subsequent frames matching after a mismatch is
> "surprising"

I tried other supported encoders like libx264 and librav1e and none of 
them report mismatches.

> 
> 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-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer March 24, 2023, 12:08 a.m. UTC | #4 
On Thu, Mar 23, 2023 at 08:32:48AM -0300, James Almer wrote:
> On 3/23/2023 6:50 AM, Michael Niedermayer wrote:
> > On Mon, Mar 20, 2023 at 07:07:59PM -0300, James Almer wrote:
> > > On 3/19/2023 4:42 PM, Michael Niedermayer wrote:
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >    libavcodec/snowenc.c | 10 +++++++++-
> > > >    1 file changed, 9 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
> > > > index 658684c575..5fb5906ed8 100644
> > > > --- a/libavcodec/snowenc.c
> > > > +++ b/libavcodec/snowenc.c
> > > > @@ -26,6 +26,7 @@
> > > >    #include "avcodec.h"
> > > >    #include "codec_internal.h"
> > > >    #include "encode.h"
> > > > +#include "internal.h" //For AVCodecInternal.recon_frame
> > > >    #include "me_cmp.h"
> > > >    #include "packet_internal.h"
> > > >    #include "snow_dwt.h"
> > > > @@ -1576,6 +1577,7 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
> > > >    {
> > > >        SnowContext *s = avctx->priv_data;
> > > >        RangeCoder * const c= &s->c;
> > > > +    AVCodecInternal *avci = avctx->internal;
> > > >        AVFrame *pic;
> > > >        const int width= s->avctx->width;
> > > >        const int height= s->avctx->height;
> > > > @@ -1877,6 +1879,10 @@ redo_frame:
> > > >                                       s->encoding_error,
> > > >                                       (s->avctx->flags&AV_CODEC_FLAG_PSNR) ? SNOW_MAX_PLANES : 0,
> > > >                                       s->current_picture->pict_type);
> > > > +    if (s->avctx->flags & AV_CODEC_FLAG_RECON_FRAME) {
> > > > +        av_frame_unref(avci->recon_frame);
> > > > +        av_frame_ref(avci->recon_frame, s->current_picture);
> > > > +    }
> > > >        pkt->size = ff_rac_terminate(c, 0);
> > > >        if (s->current_picture->key_frame)
> > > > @@ -1934,7 +1940,9 @@ const FFCodec ff_snow_encoder = {
> > > >        CODEC_LONG_NAME("Snow"),
> > > >        .p.type         = AVMEDIA_TYPE_VIDEO,
> > > >        .p.id           = AV_CODEC_ID_SNOW,
> > > > -    .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE,
> > > > +    .p.capabilities = AV_CODEC_CAP_DR1 |
> > > > +                      AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
> > > > +                      AV_CODEC_CAP_ENCODER_RECON_FRAME,
> > > >        .priv_data_size = sizeof(SnowContext),
> > > >        .init           = encode_init,
> > > >        FF_CODEC_ENCODE_CB(encode_frame),
> > > 
> > > Testing with the tool from https://git.khirnov.net/libav.git/commit/?h=recon&id=d78e17840347abfe10b2bc7e789c60665de56e1f
> > > (Which i hope will be upstreamed eventually) i get
> > > 
> > > $ tools/enc_recon_frame_test input.y4m snow flags=+bitexact 10
> > > Checksum mismatch in frame ts=0, plane 0
> > 
> > I cannot replicate this, using matrixbench and above command line i get no
> > mismatches, where can i find the input.y4m you used ?
> 
> It was https://0x0.st/sfRh.y4m
> 
> > 
> > also would be nice if this tool would write a image showing the difference
> > 
> > thx
> > 
> > > 
> > > I don't know if this really matters for snow (is it mean to be bitexact?),
> > > but it seems the very first frame returned by the reconstructed frame API is
> > > not the same as the one output by the snow decoder. The rest seem to match,
> > > though.
> > 
> > This sounds rather strange, the first frame would be used as reference for
> > motion compensation so the subsequent frames matching after a mismatch is
> > "surprising"
> 
> I tried other supported encoders like libx264 and librav1e and none of them
> report mismatches.

bugs
ill send a patchset

thx

[...]
Michael Niedermayer March 25, 2023, 8:56 p.m. UTC | #5 
On Fri, Mar 24, 2023 at 01:08:56AM +0100, Michael Niedermayer wrote:
> On Thu, Mar 23, 2023 at 08:32:48AM -0300, James Almer wrote:
> > On 3/23/2023 6:50 AM, Michael Niedermayer wrote:
> > > On Mon, Mar 20, 2023 at 07:07:59PM -0300, James Almer wrote:
> > > > On 3/19/2023 4:42 PM, Michael Niedermayer wrote:
> > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > ---
> > > > >    libavcodec/snowenc.c | 10 +++++++++-
> > > > >    1 file changed, 9 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
> > > > > index 658684c575..5fb5906ed8 100644
> > > > > --- a/libavcodec/snowenc.c
> > > > > +++ b/libavcodec/snowenc.c
> > > > > @@ -26,6 +26,7 @@
> > > > >    #include "avcodec.h"
> > > > >    #include "codec_internal.h"
> > > > >    #include "encode.h"
> > > > > +#include "internal.h" //For AVCodecInternal.recon_frame
> > > > >    #include "me_cmp.h"
> > > > >    #include "packet_internal.h"
> > > > >    #include "snow_dwt.h"
> > > > > @@ -1576,6 +1577,7 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
> > > > >    {
> > > > >        SnowContext *s = avctx->priv_data;
> > > > >        RangeCoder * const c= &s->c;
> > > > > +    AVCodecInternal *avci = avctx->internal;
> > > > >        AVFrame *pic;
> > > > >        const int width= s->avctx->width;
> > > > >        const int height= s->avctx->height;
> > > > > @@ -1877,6 +1879,10 @@ redo_frame:
> > > > >                                       s->encoding_error,
> > > > >                                       (s->avctx->flags&AV_CODEC_FLAG_PSNR) ? SNOW_MAX_PLANES : 0,
> > > > >                                       s->current_picture->pict_type);
> > > > > +    if (s->avctx->flags & AV_CODEC_FLAG_RECON_FRAME) {
> > > > > +        av_frame_unref(avci->recon_frame);
> > > > > +        av_frame_ref(avci->recon_frame, s->current_picture);
> > > > > +    }
> > > > >        pkt->size = ff_rac_terminate(c, 0);
> > > > >        if (s->current_picture->key_frame)
> > > > > @@ -1934,7 +1940,9 @@ const FFCodec ff_snow_encoder = {
> > > > >        CODEC_LONG_NAME("Snow"),
> > > > >        .p.type         = AVMEDIA_TYPE_VIDEO,
> > > > >        .p.id           = AV_CODEC_ID_SNOW,
> > > > > -    .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE,
> > > > > +    .p.capabilities = AV_CODEC_CAP_DR1 |
> > > > > +                      AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
> > > > > +                      AV_CODEC_CAP_ENCODER_RECON_FRAME,
> > > > >        .priv_data_size = sizeof(SnowContext),
> > > > >        .init           = encode_init,
> > > > >        FF_CODEC_ENCODE_CB(encode_frame),
> > > > 
> > > > Testing with the tool from https://git.khirnov.net/libav.git/commit/?h=recon&id=d78e17840347abfe10b2bc7e789c60665de56e1f
> > > > (Which i hope will be upstreamed eventually) i get
> > > > 
> > > > $ tools/enc_recon_frame_test input.y4m snow flags=+bitexact 10
> > > > Checksum mismatch in frame ts=0, plane 0
> > > 
> > > I cannot replicate this, using matrixbench and above command line i get no
> > > mismatches, where can i find the input.y4m you used ?
> > 
> > It was https://0x0.st/sfRh.y4m
> > 
> > > 
> > > also would be nice if this tool would write a image showing the difference
> > > 
> > > thx
> > > 
> > > > 
> > > > I don't know if this really matters for snow (is it mean to be bitexact?),
> > > > but it seems the very first frame returned by the reconstructed frame API is
> > > > not the same as the one output by the snow decoder. The rest seem to match,
> > > > though.
> > > 
> > > This sounds rather strange, the first frame would be used as reference for
> > > motion compensation so the subsequent frames matching after a mismatch is
> > > "surprising"
> > 
> > I tried other supported encoders like libx264 and librav1e and none of them
> > report mismatches.
> 
> bugs
> ill send a patchset

will apply the fix as well as these 3 patches here


[...]
diff mbox series

Patch

diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
index 658684c575..5fb5906ed8 100644
--- a/libavcodec/snowenc.c
+++ b/libavcodec/snowenc.c
@@ -26,6 +26,7 @@ 
 #include "avcodec.h"
 #include "codec_internal.h"
 #include "encode.h"
+#include "internal.h" //For AVCodecInternal.recon_frame
 #include "me_cmp.h"
 #include "packet_internal.h"
 #include "snow_dwt.h"
@@ -1576,6 +1577,7 @@  static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
 {
     SnowContext *s = avctx->priv_data;
     RangeCoder * const c= &s->c;
+    AVCodecInternal *avci = avctx->internal;
     AVFrame *pic;
     const int width= s->avctx->width;
     const int height= s->avctx->height;
@@ -1877,6 +1879,10 @@  redo_frame:
                                    s->encoding_error,
                                    (s->avctx->flags&AV_CODEC_FLAG_PSNR) ? SNOW_MAX_PLANES : 0,
                                    s->current_picture->pict_type);
+    if (s->avctx->flags & AV_CODEC_FLAG_RECON_FRAME) {
+        av_frame_unref(avci->recon_frame);
+        av_frame_ref(avci->recon_frame, s->current_picture);
+    }
 
     pkt->size = ff_rac_terminate(c, 0);
     if (s->current_picture->key_frame)
@@ -1934,7 +1940,9 @@  const FFCodec ff_snow_encoder = {
     CODEC_LONG_NAME("Snow"),
     .p.type         = AVMEDIA_TYPE_VIDEO,
     .p.id           = AV_CODEC_ID_SNOW,
-    .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE,
+    .p.capabilities = AV_CODEC_CAP_DR1 |
+                      AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
+                      AV_CODEC_CAP_ENCODER_RECON_FRAME,
     .priv_data_size = sizeof(SnowContext),
     .init           = encode_init,
     FF_CODEC_ENCODE_CB(encode_frame),