diff mbox

[FFmpeg-devel] avfilter/vf_tile: remove limit of max tile size

Message ID 20171027201220.3418-1-onemda@gmail.com
State Superseded
Headers show

Commit Message

Paul B Mahol Oct. 27, 2017, 8:12 p.m. UTC
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavfilter/vf_tile.c | 8 --------
 1 file changed, 8 deletions(-)

Comments

Nicolas George Oct. 27, 2017, 8:38 p.m. UTC | #1
Le sextidi 6 brumaire, an CCXXVI, Paul B Mahol a écrit :
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/vf_tile.c | 8 --------
>  1 file changed, 8 deletions(-)

Nack.

./ffmpeg_g -lavfi testsrc2=s=2x2,tile=65536x65536,scale=1024x1024 -f framecrc -

hangs instead of returning a proper error.

Regards,
Paul B Mahol Oct. 28, 2017, 7:14 a.m. UTC | #2
On 10/27/17, Nicolas George <george@nsup.org> wrote:
> Le sextidi 6 brumaire, an CCXXVI, Paul B Mahol a ecrit :
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavfilter/vf_tile.c | 8 --------
>>  1 file changed, 8 deletions(-)
>
> Nack.
>
> ./ffmpeg_g -lavfi testsrc2=s=2x2,tile=65536x65536,scale=1024x1024 -f
> framecrc -
>
> hangs instead of returning a proper error.

testsrc2 hangs here.
Nicolas George Oct. 28, 2017, 9:33 a.m. UTC | #3
Le septidi 7 brumaire, an CCXXVI, Paul B Mahol a écrit :
> testsrc2 hangs here.

So please rework your patch.

Regards,
Paul B Mahol Oct. 28, 2017, 11:38 a.m. UTC | #4
On 10/28/17, Nicolas George <george@nsup.org> wrote:
> Le septidi 7 brumaire, an CCXXVI, Paul B Mahol a ecrit :
>> testsrc2 hangs here.
>
> So please rework your patch.

Why? When it hangs without using tile filter at all.

Do I need to write things out of blue moon now?
Nicolas George Oct. 28, 2017, 11:47 a.m. UTC | #5
Le septidi 7 brumaire, an CCXXVI, Paul B Mahol a écrit :
> Why?

Because it is wrong. I wrote the test case by looking at your changes
and finding a flaw in them.
Paul B Mahol Oct. 28, 2017, 11:52 a.m. UTC | #6
On 10/28/17, Nicolas George <george@nsup.org> wrote:
> Le septidi 7 brumaire, an CCXXVI, Paul B Mahol a ecrit :
>> Why?
>
> Because it is wrong. I wrote the test case by looking at your changes
> and finding a flaw in them.

No, your test is flawed. I have no longer obligations to communicate with you.

Simply said, testsrc2 hangs on its own, and its not tile flaw, or my patch.
Clément Bœsch Oct. 28, 2017, 12:33 p.m. UTC | #7
On Fri, Oct 27, 2017 at 10:38:11PM +0200, Nicolas George wrote:
> Le sextidi 6 brumaire, an CCXXVI, Paul B Mahol a écrit :
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> >  libavfilter/vf_tile.c | 8 --------
> >  1 file changed, 8 deletions(-)
> 
> Nack.
> 
> ./ffmpeg_g -lavfi testsrc2=s=2x2,tile=65536x65536,scale=1024x1024 -f framecrc -
> 
> hangs instead of returning a proper error.
> 

In case there is confusion with what Paul said: `ffmpeg -lavfi
testsrc2=s=2x2 -f framecrc -` hangs with current git/master, so the hang
you're reporting is likely unrelated to this patch. Instead, it looks like a
problem related to the 2x2 size being too small for testsrc2 (testsrc
supports 2x2, testsrc2 doesn't).

(Not saying the current patch is correct though)
Paul B Mahol Oct. 28, 2017, 12:42 p.m. UTC | #8
On 10/28/17, Clement Boesch <u@pkh.me> wrote:
> On Fri, Oct 27, 2017 at 10:38:11PM +0200, Nicolas George wrote:
>> Le sextidi 6 brumaire, an CCXXVI, Paul B Mahol a ecrit :
>> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> > ---
>> >  libavfilter/vf_tile.c | 8 --------
>> >  1 file changed, 8 deletions(-)
>>
>> Nack.
>>
>> ./ffmpeg_g -lavfi testsrc2=s=2x2,tile=65536x65536,scale=1024x1024 -f
>> framecrc -
>>
>> hangs instead of returning a proper error.
>>
>
> In case there is confusion with what Paul said: `ffmpeg -lavfi
> testsrc2=s=2x2 -f framecrc -` hangs with current git/master, so the hang
> you're reporting is likely unrelated to this patch. Instead, it looks like
> a
> problem related to the 2x2 size being too small for testsrc2 (testsrc
> supports 2x2, testsrc2 doesn't).
>
> (Not saying the current patch is correct though)

I don't remmember I hired lawyer. Did I?
Clément Bœsch Oct. 28, 2017, 12:50 p.m. UTC | #9
On Sat, Oct 28, 2017 at 02:42:26PM +0200, Paul B Mahol wrote:
> On 10/28/17, Clement Boesch <u@pkh.me> wrote:
> > On Fri, Oct 27, 2017 at 10:38:11PM +0200, Nicolas George wrote:
> >> Le sextidi 6 brumaire, an CCXXVI, Paul B Mahol a ecrit :
> >> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> >> > ---
> >> >  libavfilter/vf_tile.c | 8 --------
> >> >  1 file changed, 8 deletions(-)
> >>
> >> Nack.
> >>
> >> ./ffmpeg_g -lavfi testsrc2=s=2x2,tile=65536x65536,scale=1024x1024 -f
> >> framecrc -
> >>
> >> hangs instead of returning a proper error.
> >>
> >
> > In case there is confusion with what Paul said: `ffmpeg -lavfi
> > testsrc2=s=2x2 -f framecrc -` hangs with current git/master, so the hang
> > you're reporting is likely unrelated to this patch. Instead, it looks like
> > a
> > problem related to the 2x2 size being too small for testsrc2 (testsrc
> > supports 2x2, testsrc2 doesn't).
> >
> > (Not saying the current patch is correct though)
> 
> I don't remmember I hired lawyer. Did I?

I'm not defending you, I'm explaining a potential misunderstanding between
Nicolas and you, giving information you should probably have provided
yourself to ease the communication to prevent pointless escalation.

Regards,
Paul B Mahol Oct. 28, 2017, 1:12 p.m. UTC | #10
On 10/28/17, Clement Boesch <u@pkh.me> wrote:
> On Sat, Oct 28, 2017 at 02:42:26PM +0200, Paul B Mahol wrote:
>> On 10/28/17, Clement Boesch <u@pkh.me> wrote:
>> > On Fri, Oct 27, 2017 at 10:38:11PM +0200, Nicolas George wrote:
>> >> Le sextidi 6 brumaire, an CCXXVI, Paul B Mahol a ecrit :
>> >> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> >> > ---
>> >> >  libavfilter/vf_tile.c | 8 --------
>> >> >  1 file changed, 8 deletions(-)
>> >>
>> >> Nack.
>> >>
>> >> ./ffmpeg_g -lavfi testsrc2=s=2x2,tile=65536x65536,scale=1024x1024 -f
>> >> framecrc -
>> >>
>> >> hangs instead of returning a proper error.
>> >>
>> >
>> > In case there is confusion with what Paul said: `ffmpeg -lavfi
>> > testsrc2=s=2x2 -f framecrc -` hangs with current git/master, so the
>> > hang
>> > you're reporting is likely unrelated to this patch. Instead, it looks
>> > like
>> > a
>> > problem related to the 2x2 size being too small for testsrc2 (testsrc
>> > supports 2x2, testsrc2 doesn't).
>> >
>> > (Not saying the current patch is correct though)
>>
>> I don't remmember I hired lawyer. Did I?
>
> I'm not defending you, I'm explaining a potential misunderstanding between
> Nicolas and you, giving information you should probably have provided
> yourself to ease the communication to prevent pointless escalation.

I already provided it. The problem is people forgot to read.
Paul B Mahol Oct. 28, 2017, 2:41 p.m. UTC | #11
On 10/28/17, Paul B Mahol <onemda@gmail.com> wrote:
> On 10/28/17, Clement Boesch <u@pkh.me> wrote:
>> On Sat, Oct 28, 2017 at 02:42:26PM +0200, Paul B Mahol wrote:
>>> On 10/28/17, Clement Boesch <u@pkh.me> wrote:
>>> > On Fri, Oct 27, 2017 at 10:38:11PM +0200, Nicolas George wrote:
>>> >> Le sextidi 6 brumaire, an CCXXVI, Paul B Mahol a ecrit :
>>> >> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>> >> > ---
>>> >> >  libavfilter/vf_tile.c | 8 --------
>>> >> >  1 file changed, 8 deletions(-)
>>> >>
>>> >> Nack.
>>> >>
>>> >> ./ffmpeg_g -lavfi testsrc2=s=2x2,tile=65536x65536,scale=1024x1024 -f
>>> >> framecrc -
>>> >>
>>> >> hangs instead of returning a proper error.
>>> >>
>>> >
>>> > In case there is confusion with what Paul said: `ffmpeg -lavfi
>>> > testsrc2=s=2x2 -f framecrc -` hangs with current git/master, so the
>>> > hang
>>> > you're reporting is likely unrelated to this patch. Instead, it looks
>>> > like
>>> > a
>>> > problem related to the 2x2 size being too small for testsrc2 (testsrc
>>> > supports 2x2, testsrc2 doesn't).
>>> >
>>> > (Not saying the current patch is correct though)
>>>
>>> I don't remmember I hired lawyer. Did I?
>>
>> I'm not defending you, I'm explaining a potential misunderstanding
>> between
>> Nicolas and you, giving information you should probably have provided
>> yourself to ease the communication to prevent pointless escalation.
>
> I already provided it. The problem is people forgot to read.
>

Gonna apply this patch soonTM because of no valid reasons to block it.
Nicolas George Oct. 28, 2017, 4 p.m. UTC | #12
Le septidi 7 brumaire, an CCXXVI, Paul B Mahol a écrit :
> Gonna apply this patch soonTM because of no valid reasons to block it.

No, you will not.
Paul B Mahol Oct. 28, 2017, 4:04 p.m. UTC | #13
On 10/28/17, Nicolas George <george@nsup.org> wrote:
> Le septidi 7 brumaire, an CCXXVI, Paul B Mahol a ecrit :
>> Gonna apply this patch soonTM because of no valid reasons to block it.
>
> No, you will not.

You still have not provided valid reason to block it.

testsrc2 filter hang is unrelated to my change.
Nicolas George Oct. 28, 2017, 4:30 p.m. UTC | #14
Le septidi 7 brumaire, an CCXXVI, Paul B Mahol a écrit :
> You still have not provided valid reason to block it.

Your first patch was wrong. I have observed a possible flaw in the
second one. I thought I had a test case to show it, maybe I was wrong. I
need to check and test.

Now, I might add, if you were not so antagonistic all the time, I might
have wasted less time in these useless exchanges and had more time to
actually examine the patch. Right now I do not have time. So you will
just have to wait.

But do not dare push the patch as is.
Paul B Mahol Oct. 28, 2017, 5:07 p.m. UTC | #15
On 10/28/17, Nicolas George <george@nsup.org> wrote:
> Le septidi 7 brumaire, an CCXXVI, Paul B Mahol a ecrit :
>> You still have not provided valid reason to block it.
>
> Your first patch was wrong. I have observed a possible flaw in the
> second one. I thought I had a test case to show it, maybe I was wrong. I
> need to check and test.
>
> Now, I might add, if you were not so antagonistic all the time, I might
> have wasted less time in these useless exchanges and had more time to
> actually examine the patch. Right now I do not have time. So you will
> just have to wait.
>
> But do not dare push the patch as is.

I already posted another version which I will push shortly.
Nicolas George Oct. 28, 2017, 6:08 p.m. UTC | #16
Le septidi 7 brumaire, an CCXXVI, Paul B Mahol a écrit :
> I already posted another version which I will push shortly.

No, you will wait until I approve the patch, since it is code that I
maintain. That is how it works.

Regards,
Paul B Mahol Oct. 28, 2017, 6:15 p.m. UTC | #17
On 10/28/17, Nicolas George <george@nsup.org> wrote:
> Le septidi 7 brumaire, an CCXXVI, Paul B Mahol a ecrit :
>> I already posted another version which I will push shortly.
>
> No, you will wait until I approve the patch, since it is code that I
> maintain. That is how it works.

No it doesn't work like that any more.

Some folks commit to maintained code by someone else without prior review.

Why I should not be one of them?
Paul B Mahol Oct. 28, 2017, 6:15 p.m. UTC | #18
On 10/28/17, Paul B Mahol <onemda@gmail.com> wrote:
> On 10/28/17, Nicolas George <george@nsup.org> wrote:
>> Le septidi 7 brumaire, an CCXXVI, Paul B Mahol a ecrit :
>>> I already posted another version which I will push shortly.
>>
>> No, you will wait until I approve the patch, since it is code that I
>> maintain. That is how it works.
>
> No it doesn't work like that any more.
>
> Some folks commit to maintained code by someone else without prior review.
>
> Why I should not be one of them?
>

And remmember, you are bad guy here.
Nicolas George Oct. 28, 2017, 6:23 p.m. UTC | #19
Le septidi 7 brumaire, an CCXXVI, Paul B Mahol a écrit :
> No it doesn't work like that any more.
> 
> Some folks commit to maintained code by someone else without prior review.
> 
> Why I should not be one of them?

Are you trying to derail the project on purpose?

Now, please behave like a civilized member of a multi-user community. I
want to review the patch to make sure it is now correct (remember it is
the third iteration, the first two were wrong), because I care about
code quality. I need time and good access to the source code for that. I
am sorry, you will have to wait. Fortunately, this patch is not about
security or a regression, it is about a harmless limitation that have
been around for years, it can wait a few days more. I am sure you have
plenty better to do with your time.

This will probably the last mail I will have time to write this
week-end; I would have preferred to spend it on something more
interesting. When I come back, I expect to find things as is, or
possibly an even better version on the patch waiting on the mailing
list. Anything else would be a declaration of open hostility.
diff mbox

Patch

diff --git a/libavfilter/vf_tile.c b/libavfilter/vf_tile.c
index 87e0b940cf..a0bfb31338 100644
--- a/libavfilter/vf_tile.c
+++ b/libavfilter/vf_tile.c
@@ -44,8 +44,6 @@  typedef struct TileContext {
     uint8_t rgba_color[4];
 } TileContext;
 
-#define REASONABLE_SIZE 1024
-
 #define OFFSET(x) offsetof(TileContext, x)
 #define FLAGS AV_OPT_FLAG_VIDEO_PARAM|AV_OPT_FLAG_FILTERING_PARAM
 
@@ -68,12 +66,6 @@  static av_cold int init(AVFilterContext *ctx)
 {
     TileContext *tile = ctx->priv;
 
-    if (tile->w > REASONABLE_SIZE || tile->h > REASONABLE_SIZE) {
-        av_log(ctx, AV_LOG_ERROR, "Tile size %ux%u is insane.\n",
-               tile->w, tile->h);
-        return AVERROR(EINVAL);
-    }
-
     if (tile->nb_frames == 0) {
         tile->nb_frames = tile->w * tile->h;
     } else if (tile->nb_frames > tile->w * tile->h) {