diff mbox

[FFmpeg-devel] avfilter/vidstab: set bytesPerPixel only for packed formats.

Message ID 61da5873-3bd9-9618-a8c8-6dd715b296e0@gmail.com
State New
Headers show

Commit Message

Gyan Dec. 24, 2017, 5:06 a.m. UTC
On 12/23/2017 4:39 PM, Gyan Doshi wrote:
> Patch for ticket #6736.

This patch alters the test, not any assignment, so corrected verbiage.

Regards,
Gyan
From dbc21f9fe4061ac30339b8086226ea2c47f8bd29 Mon Sep 17 00:00:00 2001
From: Gyan Doshi <gyandoshi@gmail.com>
Date: Sat, 23 Dec 2017 16:14:25 +0530
Subject: [PATCH] avfilter/vidstab: check bytesPerPixel only for packed
 formats.

libvidstab introduced this variable only for packed formats but in
vf_vidstab*.c, it's checked for all inputs. So the filter errors out for YUV422/444P streams.

Fixes #6736.
---
 libavfilter/vf_vidstabdetect.c    | 3 ++-
 libavfilter/vf_vidstabtransform.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Gyan Dec. 28, 2017, 4:22 a.m. UTC | #1
On 12/24/2017 10:36 AM, Gyan Doshi wrote:
> 
> On 12/23/2017 4:39 PM, Gyan Doshi wrote:
>> Patch for ticket #6736.

Ping.
Michael Niedermayer Dec. 28, 2017, 8:51 p.m. UTC | #2
On Thu, Dec 28, 2017 at 09:52:20AM +0530, Gyan Doshi wrote:
> On 12/24/2017 10:36 AM, Gyan Doshi wrote:
> >
> >On 12/23/2017 4:39 PM, Gyan Doshi wrote:
> >>Patch for ticket #6736.
> 
> Ping.

who maintains vf_vidstabtransform.c ?

[...]
Lou Logan Dec. 28, 2017, 9:50 p.m. UTC | #3
On Thu, Dec 28, 2017, at 11:51 AM, Michael Niedermayer wrote:
>
> who maintains vf_vidstabtransform.c ?

Realistically, nobody. Ideally, should have been Georg Martius who is the original author of the filter and also of vid.stab itself. Consider CCing him at "martius at mis.mpg.de" or "georg.martius at web.de".
Michael Niedermayer Dec. 29, 2017, 3:08 a.m. UTC | #4
On Thu, Dec 28, 2017 at 12:50:00PM -0900, Lou Logan wrote:
> On Thu, Dec 28, 2017, at 11:51 AM, Michael Niedermayer wrote:
> >
> > who maintains vf_vidstabtransform.c ?
> 
> Realistically, nobody. Ideally, should have been Georg Martius who is the original author of the filter and also of vid.stab itself. Consider CCing him at "martius at mis.mpg.de" or "georg.martius at web.de".

ok, cc added

Georg, do you want to maintain vf_vidstabtransform.c ?

If not, who wants to maintain vf_vidstabtransform.c ?

If noone then iam not sure who will test or apply this, i think many
dont have the dependancy installed ...

[...]
Gyan Jan. 4, 2018, 11:55 a.m. UTC | #5
On 12/29/2017 8:38 AM, Michael Niedermayer wrote:
> On Thu, Dec 28, 2017 at 12:50:00PM -0900, Lou Logan wrote:
>> On Thu, Dec 28, 2017, at 11:51 AM, Michael Niedermayer wrote:
>>>
>>> who maintains vf_vidstabtransform.c ?
>>
>> Realistically, nobody. Ideally, should have been Georg Martius who is the original author of the filter and also of vid.stab itself. Consider CCing him at "martius at mis.mpg.de" or "georg.martius at web.de".
> 
> ok, cc added
> 
> Georg, do you want to maintain vf_vidstabtransform.c ?
> 
> If not, who wants to maintain vf_vidstabtransform.c ?
> 
> If noone then iam not sure who will test or apply this, i think many
> dont have the dependancy installed ...

I assume the author hasn't gotten back yet. Can anyone else test and 
apply this?


Regards,
Gyan
Gyan Jan. 10, 2018, 4:56 a.m. UTC | #6
On 1/4/2018 5:25 PM, Gyan Doshi wrote:
> 
> On 12/29/2017 8:38 AM, Michael Niedermayer wrote:
>> On Thu, Dec 28, 2017 at 12:50:00PM -0900, Lou Logan wrote:
>>> On Thu, Dec 28, 2017, at 11:51 AM, Michael Niedermayer wrote:
>>>>
>>>> who maintains vf_vidstabtransform.c ?
>>>
>>> Realistically, nobody. Ideally, should have been Georg Martius who is 
>>> the original author of the filter and also of vid.stab itself. 
>>> Consider CCing him at "martius at mis.mpg.de" or "georg.martius at 
>>> web.de".
>>
>> ok, cc added
>>
>> Georg, do you want to maintain vf_vidstabtransform.c ?
>>
>> If not, who wants to maintain vf_vidstabtransform.c ?
>>
>> If noone then iam not sure who will test or apply this, i think many
>> dont have the dependancy installed ...
> 
> I assume the author hasn't gotten back yet. Can anyone else test and 
> apply this?

Anyone?
Michael Niedermayer Jan. 11, 2018, 12:46 a.m. UTC | #7
On Wed, Jan 10, 2018 at 10:26:58AM +0530, Gyan Doshi wrote:
> 
> 
> On 1/4/2018 5:25 PM, Gyan Doshi wrote:
> >
> >On 12/29/2017 8:38 AM, Michael Niedermayer wrote:
> >>On Thu, Dec 28, 2017 at 12:50:00PM -0900, Lou Logan wrote:
> >>>On Thu, Dec 28, 2017, at 11:51 AM, Michael Niedermayer wrote:
> >>>>
> >>>>who maintains vf_vidstabtransform.c ?
> >>>
> >>>Realistically, nobody. Ideally, should have been Georg Martius who is
> >>>the original author of the filter and also of vid.stab itself.
> >>>Consider CCing him at "martius at mis.mpg.de" or "georg.martius at
> >>>web.de".
> >>
> >>ok, cc added
> >>
> >>Georg, do you want to maintain vf_vidstabtransform.c ?
> >>
> >>If not, who wants to maintain vf_vidstabtransform.c ?
> >>
> >>If noone then iam not sure who will test or apply this, i think many
> >>dont have the dependancy installed ...
> >
> >I assume the author hasn't gotten back yet. Can anyone else test and apply
> >this?
> 
> Anyone?

If noone else cares about maintaining this ...
then there is one radical but simple solution ...
post a patch that adds you to MAINTAINERs for the file.

Anyone could object yes, but then that person should at least review and apply
the vf_vidstabtransform.c patch


[...]
wm4 Jan. 11, 2018, 12:59 a.m. UTC | #8
On Thu, 11 Jan 2018 01:46:56 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Wed, Jan 10, 2018 at 10:26:58AM +0530, Gyan Doshi wrote:
> > 
> > 
> > On 1/4/2018 5:25 PM, Gyan Doshi wrote:  
> > >
> > >On 12/29/2017 8:38 AM, Michael Niedermayer wrote:  
> > >>On Thu, Dec 28, 2017 at 12:50:00PM -0900, Lou Logan wrote:  
> > >>>On Thu, Dec 28, 2017, at 11:51 AM, Michael Niedermayer wrote:  
> > >>>>
> > >>>>who maintains vf_vidstabtransform.c ?  
> > >>>
> > >>>Realistically, nobody. Ideally, should have been Georg Martius who is
> > >>>the original author of the filter and also of vid.stab itself.
> > >>>Consider CCing him at "martius at mis.mpg.de" or "georg.martius at
> > >>>web.de".  
> > >>
> > >>ok, cc added
> > >>
> > >>Georg, do you want to maintain vf_vidstabtransform.c ?
> > >>
> > >>If not, who wants to maintain vf_vidstabtransform.c ?
> > >>
> > >>If noone then iam not sure who will test or apply this, i think many
> > >>dont have the dependancy installed ...  
> > >
> > >I assume the author hasn't gotten back yet. Can anyone else test and apply
> > >this?  
> > 
> > Anyone?  
> 
> If noone else cares about maintaining this ...
> then there is one radical but simple solution ...
> post a patch that adds you to MAINTAINERs for the file.
> 
> Anyone could object yes, but then that person should at least review and apply
> the vf_vidstabtransform.c patch

Our development policy allows applying patches in absence of the
maintainer.

That said, our MAINTAINERs file is 100% useless.
Gyan Jan. 11, 2018, 6:45 a.m. UTC | #9
On 1/11/2018 6:16 AM, Michael Niedermayer wrote:
>>>>
>>>> Georg, do you want to maintain vf_vidstabtransform.c ?
>>>>
>>>> If not, who wants to maintain vf_vidstabtransform.c ?
>>>>
>>>> If noone then iam not sure who will test or apply this, i think many
>>>> dont have the dependancy installed ...
>>>
>>> I assume the author hasn't gotten back yet. Can anyone else test and apply
>>> this?
>>
>> Anyone?
> 
> If noone else cares about maintaining this ...
> then there is one radical but simple solution ...
> post a patch that adds you to MAINTAINERs for the file.
> 
> Anyone could object yes, but then that person should at least review and apply
> the vf_vidstabtransform.c patch

I have no interest or business maintaining the library wrappers.

In the commit history for these files, I see Paul Mahol, Clément Bœsch 
and you as committers among those active. Timothy Gu has signed off as 
well, but I don't know if he's active.

So, someone among the above could take a look?

Regards,
Gyan
Michael Niedermayer Jan. 16, 2018, 11:49 a.m. UTC | #10
On Thu, Jan 11, 2018 at 12:15:00PM +0530, Gyan Doshi wrote:
> 
> On 1/11/2018 6:16 AM, Michael Niedermayer wrote:
> >>>>
> >>>>Georg, do you want to maintain vf_vidstabtransform.c ?
> >>>>
> >>>>If not, who wants to maintain vf_vidstabtransform.c ?
> >>>>
> >>>>If noone then iam not sure who will test or apply this, i think many
> >>>>dont have the dependancy installed ...
> >>>
> >>>I assume the author hasn't gotten back yet. Can anyone else test and apply
> >>>this?
> >>
> >>Anyone?
> >
> >If noone else cares about maintaining this ...
> >then there is one radical but simple solution ...
> >post a patch that adds you to MAINTAINERs for the file.
> >
> >Anyone could object yes, but then that person should at least review and apply
> >the vf_vidstabtransform.c patch
> 
> I have no interest or business maintaining the library wrappers.
> 
> In the commit history for these files, I see Paul Mahol, Clément Bœsch and
> you as committers among those active. Timothy Gu has signed off as well, but
> I don't know if he's active.
> 
> So, someone among the above could take a look?

Please correct me if i misunderstand but IIUC you basically say
you wont review patches for vf_vidstabtransform (that is what maintaining is)
but you want other people to review your patches for vf_vidstabtransform

Thats ok of course, no question, but developers might prioritize patches
from people who themselfs help reviewing others work.
Its a volunteer project after all so it depends on volunteers ...

Thanks

[...]
Gyan Jan. 16, 2018, 12:22 p.m. UTC | #11
On 1/16/2018 5:19 PM, Michael Niedermayer wrote:
> On Thu, Jan 11, 2018 at 12:15:00PM +0530, Gyan Doshi wrote:
>>
>> On 1/11/2018 6:16 AM, Michael Niedermayer wrote:
>>>>>>
>>>>>> Georg, do you want to maintain vf_vidstabtransform.c ?
>>>>>>
>>>>>> If not, who wants to maintain vf_vidstabtransform.c ?
>>>>>>
>>>>>> If noone then iam not sure who will test or apply this, i think many
>>>>>> dont have the dependancy installed ...
>>>>>
>>>>> I assume the author hasn't gotten back yet. Can anyone else test and apply
>>>>> this?
>>>>
>>>> Anyone?
>>>
>>> If noone else cares about maintaining this ...
>>> then there is one radical but simple solution ...
>>> post a patch that adds you to MAINTAINERs for the file.
>>>
>>> Anyone could object yes, but then that person should at least review and apply
>>> the vf_vidstabtransform.c patch
>>
>> I have no interest or business maintaining the library wrappers.
>>
>> In the commit history for these files, I see Paul Mahol, Clément Bœsch and
>> you as committers among those active. Timothy Gu has signed off as well, but
>> I don't know if he's active.
>>
>> So, someone among the above could take a look?
> 
> Please correct me if i misunderstand but IIUC you basically say
> you wont review patches for vf_vidstabtransform (that is what maintaining is)
> but you want other people to review your patches for vf_vidstabtransform

I won't review patches for vidstab because I can't. There is an open 
ticket which has a simple fix which I noticed, applied & tested. But I'm 
not well-versed in the internals of vidstab to be a competent maintainer 
for all other patches, as they may come up.

I believe you suggested to propose myself for maintainership *as a ruse* 
to get someone to review my patch and not in earnest. If you were, it 
would be insincere for me to go through with it.

I can vouch that the patch works at my end, and if that's not enough, it 
will still need to be tested/reviewed by someone else.

I can propose myself as a maintainer for docs if you are amenable.

Regards,
Gyan
Michael Niedermayer Jan. 17, 2018, 2:27 a.m. UTC | #12
On Tue, Jan 16, 2018 at 05:52:52PM +0530, Gyan Doshi wrote:
> 
> 
> On 1/16/2018 5:19 PM, Michael Niedermayer wrote:
> >On Thu, Jan 11, 2018 at 12:15:00PM +0530, Gyan Doshi wrote:
> >>
> >>On 1/11/2018 6:16 AM, Michael Niedermayer wrote:
> >>>>>>
> >>>>>>Georg, do you want to maintain vf_vidstabtransform.c ?
> >>>>>>
> >>>>>>If not, who wants to maintain vf_vidstabtransform.c ?
> >>>>>>
> >>>>>>If noone then iam not sure who will test or apply this, i think many
> >>>>>>dont have the dependancy installed ...
> >>>>>
> >>>>>I assume the author hasn't gotten back yet. Can anyone else test and apply
> >>>>>this?
> >>>>
> >>>>Anyone?
> >>>
> >>>If noone else cares about maintaining this ...
> >>>then there is one radical but simple solution ...
> >>>post a patch that adds you to MAINTAINERs for the file.
> >>>
> >>>Anyone could object yes, but then that person should at least review and apply
> >>>the vf_vidstabtransform.c patch
> >>
> >>I have no interest or business maintaining the library wrappers.
> >>
> >>In the commit history for these files, I see Paul Mahol, Clément Bœsch and
> >>you as committers among those active. Timothy Gu has signed off as well, but
> >>I don't know if he's active.
> >>
> >>So, someone among the above could take a look?
> >
> >Please correct me if i misunderstand but IIUC you basically say
> >you wont review patches for vf_vidstabtransform (that is what maintaining is)
> >but you want other people to review your patches for vf_vidstabtransform
> 
> I won't review patches for vidstab because I can't. There is an open ticket
> which has a simple fix which I noticed, applied & tested. But I'm not
> well-versed in the internals of vidstab to be a competent maintainer for all
> other patches, as they may come up.
> 
> I believe you suggested to propose myself for maintainership *as a ruse* to
> get someone to review my patch and not in earnest. If you were, it would be
> insincere for me to go through with it.

it was not intended "as a ruse" to get someone to review the patch. Maybe as
a "ruse" to get someone to maintain the code but really i was just trying to
write it a bit funny in hope that this would more likely get a positive reaction
from someone ...

the code really needs someone to maintain it.


> 
> I can vouch that the patch works at my end, and if that's not enough, it

ill apply it then after annother look, but i too do not have very deep
knowledge of vf_vidstabtransform. 


> will still need to be tested/reviewed by someone else.
> 

> I can propose myself as a maintainer for docs if you are amenable.

Which docs do you want to maintain ?
I think for anything thats unmaintained, a maintainer would be welcome

Thanks

[...]
Gyan Jan. 18, 2018, 5:45 a.m. UTC | #13
On 1/17/2018 7:57 AM, Michael Niedermayer wrote:

> Which docs do you want to maintain ?

There's a lot which is undocumented or not clearly so.

We regularly get Qs at Stack Exchange sites on how to do something and 
it turns out that an option has no description, an incomplete 
description, component private options are undocumented, or obsolete 
options are still shown. For the last, see example at 
http://www.ffmpeg.org/pipermail/ffmpeg-devel/2018-January/224049.html 
(patch sent in 
http://www.ffmpeg.org/pipermail/ffmpeg-devel/2018-January/224059.html)

So a survey, harmonization and clean-up of the docs is required, carried 
out, of course, in piecemeal fashion.

Regards,
Gyan

P.S. Thanks for the push on this patch.
diff mbox

Patch

diff --git a/libavfilter/vf_vidstabdetect.c b/libavfilter/vf_vidstabdetect.c
index 63a178a0c2..fd7ff3be24 100644
--- a/libavfilter/vf_vidstabdetect.c
+++ b/libavfilter/vf_vidstabdetect.c
@@ -107,10 +107,11 @@  static int config_input(AVFilterLink *inlink)
     VSMotionDetect* md = &(s->md);
     VSFrameInfo fi;
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
+    int is_planar = desc->flags & AV_PIX_FMT_FLAG_PLANAR;
 
     vsFrameInfoInit(&fi, inlink->w, inlink->h,
                     ff_av2vs_pixfmt(ctx, inlink->format));
-    if (fi.bytesPerPixel != av_get_bits_per_pixel(desc)/8) {
+    if (!is_planar && fi.bytesPerPixel != av_get_bits_per_pixel(desc)/8) {
         av_log(ctx, AV_LOG_ERROR, "pixel-format error: wrong bits/per/pixel, please report a BUG");
         return AVERROR(EINVAL);
     }
diff --git a/libavfilter/vf_vidstabtransform.c b/libavfilter/vf_vidstabtransform.c
index 11a0e3d512..d1ec1391cb 100644
--- a/libavfilter/vf_vidstabtransform.c
+++ b/libavfilter/vf_vidstabtransform.c
@@ -146,6 +146,7 @@  static int config_input(AVFilterLink *inlink)
     FILE *f;
 
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
+    int is_planar = desc->flags & AV_PIX_FMT_FLAG_PLANAR;
 
     VSTransformData *td = &(tc->td);
 
@@ -161,7 +162,7 @@  static int config_input(AVFilterLink *inlink)
         return AVERROR(EINVAL);
     }
 
-    if (fi_src.bytesPerPixel != av_get_bits_per_pixel(desc)/8 ||
+    if ((!is_planar && fi_src.bytesPerPixel != av_get_bits_per_pixel(desc)/8) ||
         fi_src.log2ChromaW != desc->log2_chroma_w ||
         fi_src.log2ChromaH != desc->log2_chroma_h) {
         av_log(ctx, AV_LOG_ERROR, "pixel-format error: bpp %i<>%i  ",