[FFmpeg-devel] avfillter/buffersrc: activate and EOF fix

Patches attached.

Paul B Mahol (12023-10-27):
> Subject: [PATCH 1/2] avfilter/buffersrc: switch to activate
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/buffersrc.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)

What would be the benefit?

>      if (s->eof)
> -        return AVERROR(EINVAL);
> +        return AVERROR_EOF;
>  

AVERROR_EOF does not have a semantic for writing, only for reading, so
no.

Regards,

Paul B Mahol (12023-10-27):
> Both patches are required to fix out of memory scenario with this use case:

Then please attach an analysis of the fix in the commit messages. Bugs
that just disappear when you rework the code completely are not fixed.

Regards,

On Fri, Oct 27, 2023 at 2:54 PM Nicolas George <george@nsup.org> wrote:

> Paul B Mahol (12023-10-27):
> > Subject: [PATCH 1/2] avfilter/buffersrc: switch to activate
> >
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> >  libavfilter/buffersrc.c | 25 ++++++++++++++++---------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
>
> What would be the benefit?
>
> >      if (s->eof)
> > -        return AVERROR(EINVAL);
> > +        return AVERROR_EOF;
> >
>
> AVERROR_EOF does not have a semantic for writing, only for reading, so
> no.
>

Both patches are required to fix out of memory scenario with this use case:

#!/usr/bin/env bash





which ffmpeg






if [ $# -lt 1 ] ; then



    echo "Usage: ${0} INPUT"



    echo "Example input can be generated with: "



    echo "  ffmpeg -v verbose -filter_complex
'testsrc2=r=25:s=640x480:d=470[out]' -map '[out]' -c:v libx264 -preset
superfast test_clip.mp4"

    exit 1



fi






INPUT="${1}"



COPY_OUTPUT="${INPUT}_copy.mp4"


IMG2_OUTPUT="${INPUT}_img2.jpg"





echo "Input: ${INPUT}"






# if you get ooms, feel free to utilize ulimit



# (ulimit -v 5000000 && )





valgrind --tool=massif --massif-out-file="massif.out.$(date -Iseconds)" \


ffmpeg \



  -y -ignore_unknown \



  -v verbose \



  -i "${INPUT}" \


    -c copy -map 0 \



    -t 470 \



    "${COPY_OUTPUT}" \



  -filter_complex
'[0:0]split=1[thumb_in];[thumb_in]trim=start=420:end=421,scale=720:-2:threads=1,setsar=1/1,hqdn3d,unsharp[thumb_out]'
\

    -map "[thumb_out]" \



    -vframes 1 \



    -f image2 \


  "${IMG2_OUTPUT}"


>
> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> 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 Fri, Oct 27, 2023 at 3:02 PM Nicolas George <george@nsup.org> wrote:

> Paul B Mahol (12023-10-27):
> > Both patches are required to fix out of memory scenario with this use
> case:
>
> Then please attach an analysis of the fix in the commit messages. Bugs
> that just disappear when you rework the code completely are not fixed.
>

It is log of 2nd patch.

Basically what happens: buffersrc keeps feeding filtergraph with more
frames after trim filter in that same filtergraph signals EOF.


>
> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> 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 Fri, Oct 27, 2023 at 3:18 PM Paul B Mahol <onemda@gmail.com> wrote:

>
>
> On Fri, Oct 27, 2023 at 3:02 PM Nicolas George <george@nsup.org> wrote:
>
>> Paul B Mahol (12023-10-27):
>> > Both patches are required to fix out of memory scenario with this use
>> case:
>>
>> Then please attach an analysis of the fix in the commit messages. Bugs
>> that just disappear when you rework the code completely are not fixed.
>>
>
> It is log of 2nd patch.
>
> Basically what happens: buffersrc keeps feeding filtergraph with more
> frames after trim filter in that same filtergraph signals EOF.
>

As OOM scenarios are bad, will apply this fix soon.


>
>
>>
>> Regards,
>>
>> --
>>   Nicolas George
>> _______________________________________________
>> 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".
>>
>

Paul B Mahol (12023-10-27):
> As OOM scenarios are bad, will apply this fix soon.

This bug, if it is a bug, has been here for quite some time already, a
few days more will not change anything. I want to analyze this properly,
do not push before please.

Regards,

On Fri, Oct 27, 2023 at 7:54 PM Nicolas George <george@nsup.org> wrote:

> Paul B Mahol (12023-10-27):
> > As OOM scenarios are bad, will apply this fix soon.
>
> This bug, if it is a bug, has been here for quite some time already, a
> few days more will not change anything. I want to analyze this properly,
> do not push before please.
>

I did proper analysis already. Pushed.


>
> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> 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".
>

Paul B Mahol (12023-10-28):
> I did proper analysis already. Pushed.

Thanks for the precedent, I will gladly do the same to you in the
future.

On Sun, Oct 29, 2023 at 10:38 AM Nicolas George <george@nsup.org> wrote:

> Paul B Mahol (12023-10-28):
> > I did proper analysis already. Pushed.
>
> Thanks for the precedent, I will gladly do the same to you in the
> future.
>

Haven't yet pushed it. This was a test. That you failed.
Still awaiting your 'proper' review.
Note that I will not wait too much.


>
> --
>   Nicolas George
> _______________________________________________
> 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".
>

Paul B Mahol (12023-10-27):
> Both patches are required to fix out of memory scenario with this use case:

I checked. The OOM still happens with these two patches => they do not
fix the issue => rejected.

> This was a test. That you failed.

Are you sure you are mature enough to go on the Internet unsupervised?

On Tue, Oct 31, 2023 at 11:55 AM Nicolas George <george@nsup.org> wrote:

> Paul B Mahol (12023-10-27):
> > Both patches are required to fix out of memory scenario with this use
> case:
>
> I checked. The OOM still happens with these two patches => they do not
> fix the issue => rejected.
>

Huh? I fixed this, and you need to give proof that this does not fixes it.
Because You can be mistaken and/or evil and simply lie.


>
> > This was a test. That you failed.
>
> Are you sure you are mature enough to go on the Internet unsupervised?
>
> --
>   Nicolas George
> _______________________________________________
> 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 Tue, Oct 31, 2023 at 9:13 PM Paul B Mahol <onemda@gmail.com> wrote:

>
>
> On Tue, Oct 31, 2023 at 11:55 AM Nicolas George <george@nsup.org> wrote:
>
>> Paul B Mahol (12023-10-27):
>> > Both patches are required to fix out of memory scenario with this use
>> case:
>>
>> I checked. The OOM still happens with these two patches => they do not
>> fix the issue => rejected.
>>
>
> Huh? I fixed this, and you need to give proof that this does not fixes it.
> Because You can be mistaken and/or evil and simply lie.
>

Also, JEEB tested it and it also fixes it for him.
So I really wonder how you tested this.


>
>
>>
>> > This was a test. That you failed.
>>
>> Are you sure you are mature enough to go on the Internet unsupervised?
>>
>> --
>>   Nicolas George
>> _______________________________________________
>> 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 Tue, Oct 31, 2023 at 12:55 PM Nicolas George <george@nsup.org> wrote:
>
> Paul B Mahol (12023-10-27):
> > Both patches are required to fix out of memory scenario with this use case:
>
> I checked. The OOM still happens with these two patches => they do not
> fix the issue => rejected.

So for me I have the following memory ballooning test case:

1. build FFmpeg with libx264 (I just haven't had the time to minimize
the test case to test other video formats)
2. Check that you have /usr/bin/time (it outputs "Maximum resident set
size" when called with -v argument)
3. Create test input with `ffmpeg -v verbose -filter_complex
'testsrc2=r=25:s=640x480:d=470[out]' -map '[out]' -c:v libx264 -preset
superfast test_clip.mp4`
4. Run the test case `/usr/bin/time -v ffmpeg -y -v verbose -i
test_clip.mp4 -c copy -map 0 -t 470 test_clip.mp4_copy.mp4
-filter_complex
"[0:0]split=1[thumb_in];[thumb_in]trim=start=420:end=421,scale=720:-2:threads=1,setsar=1/1,hqdn3d,unsharp[thumb_out]"
-map "[thumb_out]" -vframes 1 -f image2 test_clip.mp4_img2.jpg`

If the result for this is a maximum resident set size of about 60MiB
(f.ex. Maximum resident set size (kbytes): 62752), then things are how
they were before trim was switched to activate. If it is closer to
500MiB+ (f.ex. Maximum resident set size (kbytes): 609744), then that
is the memory ballooning behavior in action.

Results:

For current master the result is: bad (over 500MiB and would have kept
growing without the time limiting)
After applying the two patches: good (back to closer to 50MiB than 500MiB)

Exact steps to apply patches:
curl -L "https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20231027/8bf21777/attachment.bin"
| git am
curl -L "https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20231027/8bf21777/attachment-0001.bin"
| git am

I was planning to make a ticket for this, but Paul was too quick to
fix this so there is no trac ticket yet.

So my question is: Does this test case not improve for you after you
have applied these patches? Or are you speaking of a separate problem
which is bad both in master as well as after these patches have been
applied?

Regards,
Jan

Jan Ekström (12023-11-01):
> So my question is: Does this test case not improve for you after you
> have applied these patches? Or are you speaking of a separate problem
> which is bad both in master as well as after these patches have been
> applied?

This is the test case Paul posted yesterday (except you had the
politeness to de-script it) and I used to see that it does not fix the
issue.

Anyway, except in the simplest of cases, if a change does not include an
analysis of why the problem happens and how the change prevents it from
happening the simplest way, then it is not a bug fix, it is just dumb
luck. And most likely, the bug is not really gone, it just shifted to
not be triggered by the test case.

There is no such analysis in Paul's patches. If he can submit such an
analysis these patches can move forward. But based on my knowledge of
the activate code (I wrote it…) I am pretty certain this kind of bug
does not need a source with a single output to be switched to activate.

Regards,

Paul B Mahol (12023-10-31):
> Huh? I fixed this, and you need to give proof that this does not fixes it.

It is you wou have to prove that your patches fix anything or are in any
way useful.

> Because You can be mistaken and/or evil and simply lie.

Seriously, get a grip.

Paul B Mahol (12023-11-01):
> It fixes it for me, ffmpeg no longer keeps sending frames to EOF
> filtergraph.
> And no more memory is allocated and never freed.

And that is just LUCK because you changed unrelated things. You have to
provide a PROOF, starting with what was wrong in the original code.

> I ask you, kindly, once more time, to provide way how to trigger memory
> boom (OOM) with those patches applied.

I just ran the test case you gave in my usual testing environment. There
is nothing I can tell you more, I can just copy-paste the result.

Anyway, in the process of analyzing the bug and writing the proof for
your fix, you should be able to find out why it is still happening ins
subtly different circumstances.


ffmpeg version N-112636-g53f9d14063 Copyright (c) 2000-2023 the FFmpeg developers
  built with gcc 13 (Debian 13.2.0-5)
  configuration: --enable-shared --disable-static --enable-gpl --enable-libx264 --enable-libopus --enable-libass --enable-libfreetype --enable-opengl --assert-level=2
  libavutil      58. 31.100 / 58. 31.100
  libavcodec     60. 32.102 / 60. 32.102
  libavformat    60. 17.100 / 60. 17.100
  libavdevice    60.  4.100 / 60.  4.100
  libavfilter     9. 13.100 /  9. 13.100
  libswscale      7.  6.100 /  7.  6.100
  libswresample   4. 13.100 /  4. 13.100
  libpostproc    57.  4.100 / 57.  4.100
[Parsed_scale_2 @ 0x557c9a7e6fc0] w:720 h:-2 flags:'' interl:0
[Parsed_hqdn3d_4 @ 0x557c9a7f4640] ls:4.000000 cs:3.000000 lt:6.000000 ct:4.500000
[h264 @ 0x557c9a7e7e40] Reinit context to 640x480, pix_fmt: yuv420p
Input #0, mov,mp4,m4a,3gp,3g2,mj2, from '../test_clip.mp4':
  Metadata:
    major_brand     : isom
    minor_version   : 512
    compatible_brands: isomiso2avc1mp41
    encoder         : Lavf60.17.100
  Duration: 00:07:50.00, start: 0.000000, bitrate: 1594 kb/s
  Stream #0:0[0x1](und): Video: h264 (High), 1 reference frame (avc1 / 0x31637661), yuv420p(progressive, left), 640x480 [SAR 1:1 DAR 4:3], 1592 kb/s, 25 fps, 25 tbr, 12800 tbn (default)
    Metadata:
      handler_name    : VideoHandler
      vendor_id       : [0][0][0][0]
      encoder         : Lavc60.32.102 libx264
[out#0/mp4 @ 0x557c9b07e000] Adding streams from explicit maps...
[vost#0:0/copy @ 0x557c9b07f080] Created video stream from input stream 0:0
Output #0, mp4, to '../test_clip.mp4_copy.mp4':
  Metadata:
    major_brand     : isom
    minor_version   : 512
    compatible_brands: isomiso2avc1mp41
    encoder         : Lavf60.17.100
  Stream #0:0(und): Video: h264 (High), 1 reference frame (avc1 / 0x31637661), yuv420p(progressive, left), 640x480 (0x0) [SAR 1:1 DAR 4:3], q=2-31, 1592 kb/s, 25 fps, 25 tbr, 12800 tbn (default)
    Metadata:
      handler_name    : VideoHandler
      vendor_id       : [0][0][0][0]
      encoder         : Lavc60.32.102 libx264
[out#1/image2 @ 0x557c9b0c1640] Adding streams from explicit maps...
[out#1/image2 @ 0x557c9b0c1640] Creating output stream from an explicitly mapped complex filtergraph 0, output [thumb_out]
[vost#1:0/mjpeg @ 0x557c9b0c44c0] Created video stream from complex filtergraph 0:[unsharp:default]
[vost#1:0/mjpeg @ 0x557c9b0c44c0]
Stream mapping:
  Stream #0:0 (h264) -> split:default
  Stream #0:0 -> #0:0 (copy)
  unsharp:default -> Stream #1:0 (mjpeg)
Press [q] to stop, [?] for help
[h264 @ 0x557c9a8099c0] Reinit context to 640x480, pix_fmt: yuv420p
[Parsed_scale_2 @ 0x557c9a80fa80] w:720 h:-2 flags:'' interl:0
[Parsed_hqdn3d_4 @ 0x557c9a7e4180] ls:4.000000 cs:3.000000 lt:6.000000 ct:4.500000
[graph 0 input from stream 0:0 @ 0x557c9b0d9dc0] w:640 h:480 pixfmt:yuv420p tb:1/12800 fr:25/1 sar:1/1
[swscaler @ 0x557c9b0ddb00] deprecated pixel format used, make sure you did set range correctly
[Parsed_scale_2 @ 0x557c9a80fa80] w:640 h:480 fmt:yuv420p sar:1/1 -> w:720 h:540 fmt:yuvj420p sar:1/1 flags:0x00000004
[Parsed_setsar_3 @ 0x557c9a80dcc0] w:720 h:540 sar:1/1 dar:4/3 -> sar:1/1 dar:4/3
[Parsed_unsharp_5 @ 0x557c9a80fdc0] effect:sharpen type:luma msize_x:5 msize_y:5 amount:1.00
[Parsed_unsharp_5 @ 0x557c9a80fdc0] effect:none type:chroma msize_x:5 msize_y:5 amount:0.00
[vost#1:0/mjpeg @ 0x557c9b0c44c0] *** 10500 dup!
[vost#1:0/mjpeg @ 0x557c9b0c44c0] More than 1000 frames duplicated
Output #1, image2, to 'test_clip.mp4_img2.jpg':
  Metadata:
    major_brand     : isom
    minor_version   : 512
    compatible_brands: isomiso2avc1mp41
    encoder         : Lavf60.17.100
  Stream #1:0: Video: mjpeg, 1 reference frame, yuvj420p(pc, progressive, left), 720x540 (0x0) [SAR 1:1 DAR 4:3], q=2-31, 200 kb/s, 25 fps, 25 tbn
    Metadata:
      encoder         : Lavc60.32.102 mjpeg
    Side data:
      cpb: bitrate max/min/avg: 0/0/200000 buffer size: 0 vbv_delay: N/A
Error while filtering: Resource temporarily unavailable
[vist#0:0/h264 @ 0x557c9a833f00] Decoder thread received EOF packet
[vist#0:0/h264 @ 0x557c9a833f00] Decoder returned EOF, finishing
[vist#0:0/h264 @ 0x557c9a833f00] Terminating decoder thread
[out#0/mp4 @ 0x557c9b07e000] All streams finished
[out#0/mp4 @ 0x557c9b07e000] Terminating muxer thread
[AVIOContext @ 0x557c9b0c0000] Statistics: 83790919 bytes written, 2 seeks, 323 writeouts
[out#0/mp4 @ 0x557c9b07e000] Output file #0 (../test_clip.mp4_copy.mp4):
[out#0/mp4 @ 0x557c9b07e000]   Output stream #0:0 (video): 10511 packets muxed (83686326 bytes);
[out#0/mp4 @ 0x557c9b07e000]   Total: 10511 packets (83686326 bytes) muxed
[out#0/mp4 @ 0x557c9b07e000] video:81725kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 0.124977%
[out#1/image2 @ 0x557c9b0c1640] Nothing was written into output file, because at least one of its streams received no packets.
frame=    0 fps=0.0 q=0.0 Lsize=   81827kB time=00:07:00.32 bitrate=1594.8kbits/s dup=10500 drop=0 speed= 142x
[in#0/mov,mp4,m4a,3gp,3g2,mj2 @ 0x557c9a7e72c0] Terminating demuxer thread
[in#0/mov,mp4,m4a,3gp,3g2,mj2 @ 0x557c9a7e72c0] Input file #0 (../test_clip.mp4):
[in#0/mov,mp4,m4a,3gp,3g2,mj2 @ 0x557c9a7e72c0]   Input stream #0:0 (video): 10513 packets read (83701450 bytes); 10511 frames decoded; 0 decode errors;
[in#0/mov,mp4,m4a,3gp,3g2,mj2 @ 0x557c9a7e72c0]   Total: 10513 packets (83701450 bytes) demuxed
[AVIOContext @ 0x557c9a7e5d40] Statistics: 83871773 bytes read, 2 seeks
Conversion failed!

On Wed, Nov 1, 2023 at 2:58 PM Nicolas George <george@nsup.org> wrote:

> Paul B Mahol (12023-10-31):
> > Huh? I fixed this, and you need to give proof that this does not fixes
> it.
>
> It is you wou have to prove that your patches fix anything or are in any
> way useful.
>

It fixes it for me, ffmpeg no longer keeps sending frames to EOF
filtergraph.
And no more memory is allocated and never freed.

I ask you, kindly, once more time, to provide way how to trigger memory
boom (OOM) with those patches applied.


>
> > Because You can be mistaken and/or evil and simply lie.
>
> Seriously, get a grip.


> --
>   Nicolas George
> _______________________________________________
> 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 11/1/2023 10:56 AM, Nicolas George wrote:
> Jan Ekström (12023-11-01):
>> So my question is: Does this test case not improve for you after you
>> have applied these patches? Or are you speaking of a separate problem
>> which is bad both in master as well as after these patches have been
>> applied?
> 
> This is the test case Paul posted yesterday (except you had the
> politeness to de-script it) and I used to see that it does not fix the
> issue.

Can you be more specific? Do you not see the result Jan described? Do 
you see something different? Or is it that you do see what he described, 
but it's not a proper fix in your opinion as Paul's patch simply 
prevents the leak from being triggered in his specific testcase?

> 
> Anyway, except in the simplest of cases, if a change does not include an
> analysis of why the problem happens and how the change prevents it from
> happening the simplest way, then it is not a bug fix, it is just dumb
> luck. And most likely, the bug is not really gone, it just shifted to
> not be triggered by the test case.
> 
> There is no such analysis in Paul's patches. If he can submit such an
> analysis these patches can move forward. But based on my knowledge of
> the activate code (I wrote it…) I am pretty certain this kind of bug
> does not need a source with a single output to be switched to activate.
> 
> Regards,
> 
> 
> _______________________________________________
> 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".

James Almer (12023-11-01):
> Can you be more specific? Do you not see the result Jan described? Do you
> see something different? Or is it that you do see what he described, but
> it's not a proper fix in your opinion as Paul's patch simply prevents the
> leak from being triggered in his specific testcase?

I posted my console output in the last mail.

And even if it did make the bug go away, such an important change is not
acceptable without the proof that it is necessary in the commit message.

Regards,

Paul B Mahol (12023-11-02):
> You applied both patches? At correct order?

Duh.

> I probably should merge patches into single one.

You need to analyze the bug and produce a proof that it is correct. A
change to activate is not acceptable without a proof that it is
necessary. The proof should be detailed and in the commit message.

On Wed, Nov 1, 2023 at 3:13 PM Nicolas George <george@nsup.org> wrote:

> Paul B Mahol (12023-11-01):
> > It fixes it for me, ffmpeg no longer keeps sending frames to EOF
> > filtergraph.
> > And no more memory is allocated and never freed.
>
> And that is just LUCK because you changed unrelated things. You have to
> provide a PROOF, starting with what was wrong in the original code.
>
> > I ask you, kindly, once more time, to provide way how to trigger memory
> > boom (OOM) with those patches applied.
>
> I just ran the test case you gave in my usual testing environment. There
> is nothing I can tell you more, I can just copy-paste the result.
>
> Anyway, in the process of analyzing the bug and writing the proof for
> your fix, you should be able to find out why it is still happening ins
> subtly different circumstances.
>
>
> ffmpeg version N-112636-g53f9d14063 Copyright (c) 2000-2023 the FFmpeg
> developers
>   built with gcc 13 (Debian 13.2.0-5)
>   configuration: --enable-shared --disable-static --enable-gpl
> --enable-libx264 --enable-libopus --enable-libass --enable-libfreetype
> --enable-opengl --assert-level=2
>   libavutil      58. 31.100 / 58. 31.100
>   libavcodec     60. 32.102 / 60. 32.102
>   libavformat    60. 17.100 / 60. 17.100
>   libavdevice    60.  4.100 / 60.  4.100
>   libavfilter     9. 13.100 /  9. 13.100
>   libswscale      7.  6.100 /  7.  6.100
>   libswresample   4. 13.100 /  4. 13.100
>   libpostproc    57.  4.100 / 57.  4.100
> [Parsed_scale_2 @ 0x557c9a7e6fc0] w:720 h:-2 flags:'' interl:0
> [Parsed_hqdn3d_4 @ 0x557c9a7f4640] ls:4.000000 cs:3.000000 lt:6.000000
> ct:4.500000
> [h264 @ 0x557c9a7e7e40] Reinit context to 640x480, pix_fmt: yuv420p
> Input #0, mov,mp4,m4a,3gp,3g2,mj2, from '../test_clip.mp4':
>   Metadata:
>     major_brand     : isom
>     minor_version   : 512
>     compatible_brands: isomiso2avc1mp41
>     encoder         : Lavf60.17.100
>   Duration: 00:07:50.00, start: 0.000000, bitrate: 1594 kb/s
>   Stream #0:0[0x1](und): Video: h264 (High), 1 reference frame (avc1 /
> 0x31637661), yuv420p(progressive, left), 640x480 [SAR 1:1 DAR 4:3], 1592
> kb/s, 25 fps, 25 tbr, 12800 tbn (default)
>     Metadata:
>       handler_name    : VideoHandler
>       vendor_id       : [0][0][0][0]
>       encoder         : Lavc60.32.102 libx264
> [out#0/mp4 @ 0x557c9b07e000] Adding streams from explicit maps...
> [vost#0:0/copy @ 0x557c9b07f080] Created video stream from input stream 0:0
> Output #0, mp4, to '../test_clip.mp4_copy.mp4':
>   Metadata:
>     major_brand     : isom
>     minor_version   : 512
>     compatible_brands: isomiso2avc1mp41
>     encoder         : Lavf60.17.100
>   Stream #0:0(und): Video: h264 (High), 1 reference frame (avc1 /
> 0x31637661), yuv420p(progressive, left), 640x480 (0x0) [SAR 1:1 DAR 4:3],
> q=2-31, 1592 kb/s, 25 fps, 25 tbr, 12800 tbn (default)
>     Metadata:
>       handler_name    : VideoHandler
>       vendor_id       : [0][0][0][0]
>       encoder         : Lavc60.32.102 libx264
> [out#1/image2 @ 0x557c9b0c1640] Adding streams from explicit maps...
> [out#1/image2 @ 0x557c9b0c1640] Creating output stream from an explicitly
> mapped complex filtergraph 0, output [thumb_out]
> [vost#1:0/mjpeg @ 0x557c9b0c44c0] Created video stream from complex
> filtergraph 0:[unsharp:default]
> [vost#1:0/mjpeg @ 0x557c9b0c44c0]
> Stream mapping:
>   Stream #0:0 (h264) -> split:default
>   Stream #0:0 -> #0:0 (copy)
>   unsharp:default -> Stream #1:0 (mjpeg)
> Press [q] to stop, [?] for help
> [h264 @ 0x557c9a8099c0] Reinit context to 640x480, pix_fmt: yuv420p
> [Parsed_scale_2 @ 0x557c9a80fa80] w:720 h:-2 flags:'' interl:0
> [Parsed_hqdn3d_4 @ 0x557c9a7e4180] ls:4.000000 cs:3.000000 lt:6.000000
> ct:4.500000
> [graph 0 input from stream 0:0 @ 0x557c9b0d9dc0] w:640 h:480
> pixfmt:yuv420p tb:1/12800 fr:25/1 sar:1/1
> [swscaler @ 0x557c9b0ddb00] deprecated pixel format used, make sure you
> did set range correctly
> [Parsed_scale_2 @ 0x557c9a80fa80] w:640 h:480 fmt:yuv420p sar:1/1 -> w:720
> h:540 fmt:yuvj420p sar:1/1 flags:0x00000004
> [Parsed_setsar_3 @ 0x557c9a80dcc0] w:720 h:540 sar:1/1 dar:4/3 -> sar:1/1
> dar:4/3
> [Parsed_unsharp_5 @ 0x557c9a80fdc0] effect:sharpen type:luma msize_x:5
> msize_y:5 amount:1.00
> [Parsed_unsharp_5 @ 0x557c9a80fdc0] effect:none type:chroma msize_x:5
> msize_y:5 amount:0.00
> [vost#1:0/mjpeg @ 0x557c9b0c44c0] *** 10500 dup!
> [vost#1:0/mjpeg @ 0x557c9b0c44c0] More than 1000 frames duplicated
> Output #1, image2, to 'test_clip.mp4_img2.jpg':
>   Metadata:
>     major_brand     : isom
>     minor_version   : 512
>     compatible_brands: isomiso2avc1mp41
>     encoder         : Lavf60.17.100
>   Stream #1:0: Video: mjpeg, 1 reference frame, yuvj420p(pc, progressive,
> left), 720x540 (0x0) [SAR 1:1 DAR 4:3], q=2-31, 200 kb/s, 25 fps, 25 tbn
>     Metadata:
>       encoder         : Lavc60.32.102 mjpeg
>     Side data:
>       cpb: bitrate max/min/avg: 0/0/200000 buffer size: 0 vbv_delay: N/A
> Error while filtering: Resource temporarily unavailable
> [vist#0:0/h264 @ 0x557c9a833f00] Decoder thread received EOF packet
> [vist#0:0/h264 @ 0x557c9a833f00] Decoder returned EOF, finishing
> [vist#0:0/h264 @ 0x557c9a833f00] Terminating decoder thread
> [out#0/mp4 @ 0x557c9b07e000] All streams finished
> [out#0/mp4 @ 0x557c9b07e000] Terminating muxer thread
> [AVIOContext @ 0x557c9b0c0000] Statistics: 83790919 bytes written, 2
> seeks, 323 writeouts
> [out#0/mp4 @ 0x557c9b07e000] Output file #0 (../test_clip.mp4_copy.mp4):
> [out#0/mp4 @ 0x557c9b07e000]   Output stream #0:0 (video): 10511 packets
> muxed (83686326 bytes);
> [out#0/mp4 @ 0x557c9b07e000]   Total: 10511 packets (83686326 bytes) muxed
> [out#0/mp4 @ 0x557c9b07e000] video:81725kB audio:0kB subtitle:0kB other
> streams:0kB global headers:0kB muxing overhead: 0.124977%
> [out#1/image2 @ 0x557c9b0c1640] Nothing was written into output file,
> because at least one of its streams received no packets.
> frame=    0 fps=0.0 q=0.0 Lsize=   81827kB time=00:07:00.32
> bitrate=1594.8kbits/s dup=10500 drop=0 speed= 142x
> [in#0/mov,mp4,m4a,3gp,3g2,mj2 @ 0x557c9a7e72c0] Terminating demuxer thread
> [in#0/mov,mp4,m4a,3gp,3g2,mj2 @ 0x557c9a7e72c0] Input file #0
> (../test_clip.mp4):
> [in#0/mov,mp4,m4a,3gp,3g2,mj2 @ 0x557c9a7e72c0]   Input stream #0:0
> (video): 10513 packets read (83701450 bytes); 10511 frames decoded; 0
> decode errors;
> [in#0/mov,mp4,m4a,3gp,3g2,mj2 @ 0x557c9a7e72c0]   Total: 10513 packets
> (83701450 bytes) demuxed
> [AVIOContext @ 0x557c9a7e5d40] Statistics: 83871773 bytes read, 2 seeks
> Conversion failed!
>

You applied both patches? At correct order?
I probably should merge patches into single one.


>
> --
>   Nicolas George
> _______________________________________________
> 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 Thu, Nov 2, 2023 at 10:50 AM Nicolas George <george@nsup.org> wrote:

> Paul B Mahol (12023-11-02):
> > You applied both patches? At correct order?
>
> Duh.
>

There are two  patches, OOM is fixed only if both are applied.


>
> > I probably should merge patches into single one.
>
> You need to analyze the bug and produce a proof that it is correct. A
> change to activate is not acceptable without a proof that it is
> necessary. The proof should be detailed and in the commit message.
>

I analyzed it already, It is very sorry state of libavfilter that buffersrc
keeps sending frames to EOF filtergraph.


>
> --
>   Nicolas George
> _______________________________________________
> 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".
>

Paul B Mahol (12023-11-02):
> I analyzed it already, It is very sorry state of libavfilter that buffersrc
> keeps sending frames to EOF filtergraph.

So, Paul, I will explain this to you one last time.

Remember high school, when you had math test, and if you just gave the
result you got almost no points even if the result was correct? That was
because solving a math exercise is not just about finding the result, it
is about showing how you reach the result and proving that the result is
correct.

The same goes here. On your own projects, you can change randomly the
code until a bug is no longer triggered and call it fixed.

But in FFmpeg, you are not alone, and when a change is not trivial you
have to prove to your fellow developers that it is correct and
necessary.

So get to work, produce that proof, re-submit the patch with the proof
in the commit message, and then we can talk.

As it is, the need to switch buffersrc to activate is not established,
and therefore it should not be done.

Paul B Mahol (12023-11-02):
> I do not understand. What proof you need?

That the patch is correct and necessary.

First, explain how the current triggers the problem. In the commit
message.

On Thu, Nov 2, 2023 at 11:03 AM Nicolas George <george@nsup.org> wrote:

> Paul B Mahol (12023-11-02):
> > I analyzed it already, It is very sorry state of libavfilter that
> buffersrc
> > keeps sending frames to EOF filtergraph.
>
> So, Paul, I will explain this to you one last time.
>
> Remember high school, when you had math test, and if you just gave the
> result you got almost no points even if the result was correct? That was
> because solving a math exercise is not just about finding the result, it
> is about showing how you reach the result and proving that the result is
> correct.
>
> The same goes here. On your own projects, you can change randomly the
> code until a bug is no longer triggered and call it fixed.
>
> But in FFmpeg, you are not alone, and when a change is not trivial you
> have to prove to your fellow developers that it is correct and
> necessary.
>
> So get to work, produce that proof, re-submit the patch with the proof
> in the commit message, and then we can talk.
>
> As it is, the need to switch buffersrc to activate is not established,
> and therefore it should not be done.
>

I do not understand. What proof you need?
Have you even tested this JEEB script without patches applied?
It should straight forward cause OOM bomb.

Have you noticed that buffersrc filter never checks outlink status of its
output link?


>
> --
>   Nicolas George
> _______________________________________________
> 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 Thu, Nov 2, 2023 at 6:10 AM Paul B Mahol <onemda@gmail.com> wrote:
>
> On Thu, Nov 2, 2023 at 11:03 AM Nicolas George <george@nsup.org> wrote:
>
> > Paul B Mahol (12023-11-02):
> > > I analyzed it already, It is very sorry state of libavfilter that
> > buffersrc
> > > keeps sending frames to EOF filtergraph.
> >
> > So, Paul, I will explain this to you one last time.
> >
> > Remember high school, when you had math test, and if you just gave the
> > result you got almost no points even if the result was correct? That was
> > because solving a math exercise is not just about finding the result, it
> > is about showing how you reach the result and proving that the result is
> > correct.
> >
> > The same goes here. On your own projects, you can change randomly the
> > code until a bug is no longer triggered and call it fixed.
> >
> > But in FFmpeg, you are not alone, and when a change is not trivial you
> > have to prove to your fellow developers that it is correct and
> > necessary.
> >
> > So get to work, produce that proof, re-submit the patch with the proof
> > in the commit message, and then we can talk.
> >
> > As it is, the need to switch buffersrc to activate is not established,
> > and therefore it should not be done.
> >
>
> I do not understand. What proof you need?
> Have you even tested this JEEB script without patches applied?
> It should straight forward cause OOM bomb.
>
> Have you noticed that buffersrc filter never checks outlink status of its
> output link?
>

Just to confirm that I can reproduce JEEB's test, before the patches:

    Maximum resident set size (kbytes): 629568

with *both* patches applied:

    Maximum resident set size (kbytes): 80920

Best,
Tristan

>
> >
> > --
> >   Nicolas George
> > _______________________________________________
> > 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".
> >
> _______________________________________________
> 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".

Tristan Matthews (12023-11-02):
> Just to confirm that I can reproduce JEEB's test, before the patches:

Just to be clear: I never doubted that Paul's patches do make the bug go
away in your test case. That would imply accusing Paul of lying about
simple technical facts, that would be both stupid and uncivil.

What you need to understand is that is barely relevant: “I churned the
code a lot and now the bug does not happen in my test case” is not an
acceptable way of leading development.

What Paul needs to provide (or you, or anybody else; no idea who “JEEB”
is) is a proof that the changes are both correct and necessary.

Correct: you need to prove that this bug is fixed in all cases, not just
the one you were testing on.

Necessary: you need to establish that the bug cannot be fixed with fewer
changes, because each change is chance for introducing a new bug.

Such a proof would certainly start with with an analysis of how the bug
gets triggered by the current code.

The “necessary” point is especially important: it is a well-established
principle that filters with no more than one input and one output do not
need to use the activate framework directly (and therefore should not,
as it makes the code more complex).

If Paul thinks he found an exception to that well-established law, he
needs to present strong evidence.

But if no such exception exist (which is the most likely: the law is
true), then it is entirely possible that what Paul found is a bug in the
code that implements the activate mechanism for simple filters. And in
that case, we need to fix that bug.

Or maybe the actual problem is somewhere in the fftools an error check
is skipped.

We need to know what is happening before any fix can be devised. We need
an analysis of the bug. Any effort going into this that is not first
analyzing what is going on is a waste of time.

Regards,

On Fri, Nov 3, 2023 at 8:04 PM Nicolas George <george@nsup.org> wrote:

> Tristan Matthews (12023-11-02):
> > Just to confirm that I can reproduce JEEB's test, before the patches:
>
> Just to be clear: I never doubted that Paul's patches do make the bug go
> away in your test case. That would imply accusing Paul of lying about
> simple technical facts, that would be both stupid and uncivil.
>
> What you need to understand is that is barely relevant: “I churned the
> code a lot and now the bug does not happen in my test case” is not an
> acceptable way of leading development.
>
> What Paul needs to provide (or you, or anybody else; no idea who “JEEB”
> is) is a proof that the changes are both correct and necessary.
>
> Correct: you need to prove that this bug is fixed in all cases, not just
> the one you were testing on.
>
> Necessary: you need to establish that the bug cannot be fixed with fewer
> changes, because each change is chance for introducing a new bug.
>
> Such a proof would certainly start with with an analysis of how the bug
> gets triggered by the current code.
>
> The “necessary” point is especially important: it is a well-established
> principle that filters with no more than one input and one output do not
> need to use the activate framework directly (and therefore should not,
> as it makes the code more complex).
>
> If Paul thinks he found an exception to that well-established law, he
> needs to present strong evidence.
>
> But if no such exception exist (which is the most likely: the law is
> true), then it is entirely possible that what Paul found is a bug in the
> code that implements the activate mechanism for simple filters. And in
> that case, we need to fix that bug.
>
> Or maybe the actual problem is somewhere in the fftools an error check
> is skipped.
>
> We need to know what is happening before any fix can be devised. We need
> an analysis of the bug. Any effort going into this that is not first
> analyzing what is going on is a waste of time.
>

What an triptych of text for such simple concepts, Homer and Tolstoy would
be embarrassed.

Introduction of .activate API introduced checking for explicit EOF from
both direction in filter processing,
the next filter in filtergraph, and for filter previous of current filter
in filtergraph.
buffersrc filter patch for switching to activate adds explicit code to
check for EOF reached in forward (relative to buffersrc filter) direction
of filtergraph processing.
I'm not aware that buffersrc ever checked for such cases even in era before
.activate API was introduced.

By simply adding printf into buffersrc filter code or adding showinfo
filter between buffersrc and next filter (relative from buffersrc filter)
one can find out that buffersrc ignores
EOF and keeps pushing frames to next filters in filtergraph that reached
EOF status.

Also I think that forward and/or backward EOF direction status checking is
not correctly handled at all for any filters not using .activate(), and I'm
not aware that it was ever working correctly in all cases.


>
> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> 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".
>

Paul B Mahol (12023-11-03):
> Also I think that forward and/or backward EOF direction status checking is
> not correctly handled at all for any filters not using .activate(), and I'm
> not aware that it was ever working correctly in all cases.

Could you not start with that?!?

If you are right, that means there is a bug in the framework code. Then
the patch forward is to fix that bug, not needlessly single every simple
filter to activate.