[FFmpeg-devel,1/2] libavfilter/scale: More descriptive in/ref/out logging

Submitted by Kevin Mark on June 5, 2017, 10:55 a.m.

Details

Message ID 20170605105521.45420-1-kmark937@gmail.com
State New
Headers show

Commit Message

Kevin Mark June 5, 2017, 10:55 a.m.
This change makes it more clear when using the scale and scale2ref
filters what is actually happening. The old format did not
differentiate between scale and scale2ref which would make it seem
that, when using scale2ref, the ref was what was truly being scaled.

Old format for both scale and scale2ref:

w:640 h:360 fmt:rgb24 sar:1/1 -> w:160 h:120 fmt:rgb24 sar:4/3 flags:0x2

The left side is the input and the right side is the output. While
this is sufficiently clear for scale, for scale2ref it appears to
conflate the main input with the reference input. To be fair that is
exactly what the code is doing (and on purpose) but that's not a very
intuitive implementation detail to expose to the user. Now that the
main input's constants are exposed in scale2ref it makes even more
sense to correct this.

New format for scale:

in  w:320 h:240 fmt:rgb24 sar:1/1
out w:80 h:60 fmt:rgb24 sar:1/1 flags:0xc0000

New format for scale2ref:

in  w:320 h:240 fmt:rgb24 sar:1/1
ref w:640 h:360 fmt:rgb24 sar:1/1
out w:160 h:120 fmt:rgb24 sar:4/3 flags:0x2

The increase in clarity is self-evident.

Signed-off-by: Kevin Mark <kmark937@gmail.com>
---
 libavfilter/vf_scale.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Michael Niedermayer June 6, 2017, 3:49 p.m.
On Mon, Jun 05, 2017 at 06:55:20AM -0400, Kevin Mark wrote:
> This change makes it more clear when using the scale and scale2ref
> filters what is actually happening. The old format did not
> differentiate between scale and scale2ref which would make it seem
> that, when using scale2ref, the ref was what was truly being scaled.
> 
> Old format for both scale and scale2ref:
> 
> w:640 h:360 fmt:rgb24 sar:1/1 -> w:160 h:120 fmt:rgb24 sar:4/3 flags:0x2
> 
> The left side is the input and the right side is the output. While
> this is sufficiently clear for scale, for scale2ref it appears to
> conflate the main input with the reference input. To be fair that is
> exactly what the code is doing (and on purpose) but that's not a very
> intuitive implementation detail to expose to the user. Now that the
> main input's constants are exposed in scale2ref it makes even more
> sense to correct this.
> 
> New format for scale:
> 
> in  w:320 h:240 fmt:rgb24 sar:1/1
> out w:80 h:60 fmt:rgb24 sar:1/1 flags:0xc0000
> 
> New format for scale2ref:
> 
> in  w:320 h:240 fmt:rgb24 sar:1/1
> ref w:640 h:360 fmt:rgb24 sar:1/1
> out w:160 h:120 fmt:rgb24 sar:4/3 flags:0x2
> 
> The increase in clarity is self-evident.

yes but its much harder to grep for as its not a single line anymore

[...]
Kevin Mark June 6, 2017, 8:17 p.m.
On Tue, Jun 6, 2017 at 11:49 AM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> yes but its much harder to grep for as its not a single line anymore

I agree that it's not going to be as pretty a regular expression to
grep through, as there is 33% more data, but it should still be doable
without too much effort. How important is it that we maintain "API"
compatibility on verbose CLI output?

ffmpeg [...] scale2ref=0:0 [...] -v verbose - 2>&1 >/dev/null | grep -oP 'regex'

Where regex is:

(in|out|ref) +w:(\d+) h:(\d+) fmt:(\w+) sar:(\d+)\/(\d+)(?:
flags:0x[[:xdigit:]]+)?

Assuming GNU grep 2.25+, you'll get:

in  w:320 h:240 fmt:rgb24 sar:1/1
ref w:640 h:360 fmt:rgb24 sar:1/1
out w:640 h:360 fmt:rgb24 sar:3/4 flags:0x2

It also works with BSD grep 2.5.1-FreeBSD included in macOS if you use
the -E option instead of -P. These would be considered three separate
matches so if you're using a good regex engine it'd be pretty easy to
loop over each match, check the first group to determine if it's in,
ref, or out and act accordingly on the rest of the captured data. You
could also, if you wanted, assume that the first line is in and the
second line is out if you only have two matches (or lines even) and if
you have three matches/lines the first is in, second is ref, third is
out. If you needed it to work with less sophisticated engines it
shouldn't be too hard to dumb down the regex above.

Live-ish example: https://regex101.com/r/wvHLpa/1

Is there a special property that makes single lines much easier to
grep? Something specific to bash? I wouldn't think bash would have any
problems looping over this by line.

Thanks,
Kevin
Kevin Mark June 12, 2017, 4:59 a.m.
Hi Michael,

Hoping to get your thoughts on the grepability issue (wrt my previous
email). If it needs to be on a single line there's no reason the new format
cannot be changed to do so (removing the \n and adding a separator,
really). However I'm a big fan of it as-is (for both scale and scale2ref)
and I can't find a case of my own where the regex I proposed would be
troublesome.

Thanks,
Kevin

On Tue, Jun 6, 2017 at 4:17 PM Kevin Mark <kmark937@gmail.com> wrote:

> On Tue, Jun 6, 2017 at 11:49 AM, Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> > yes but its much harder to grep for as its not a single line anymore
>
> I agree that it's not going to be as pretty a regular expression to
> grep through, as there is 33% more data, but it should still be doable
> without too much effort. How important is it that we maintain "API"
> compatibility on verbose CLI output?
>
> ffmpeg [...] scale2ref=0:0 [...] -v verbose - 2>&1 >/dev/null | grep -oP
> 'regex'
>
> Where regex is:
>
> (in|out|ref) +w:(\d+) h:(\d+) fmt:(\w+) sar:(\d+)\/(\d+)(?:
> flags:0x[[:xdigit:]]+)?
>
> Assuming GNU grep 2.25+, you'll get:
>
> in  w:320 h:240 fmt:rgb24 sar:1/1
> ref w:640 h:360 fmt:rgb24 sar:1/1
> out w:640 h:360 fmt:rgb24 sar:3/4 flags:0x2
>
> It also works with BSD grep 2.5.1-FreeBSD included in macOS if you use
> the -E option instead of -P. These would be considered three separate
> matches so if you're using a good regex engine it'd be pretty easy to
> loop over each match, check the first group to determine if it's in,
> ref, or out and act accordingly on the rest of the captured data. You
> could also, if you wanted, assume that the first line is in and the
> second line is out if you only have two matches (or lines even) and if
> you have three matches/lines the first is in, second is ref, third is
> out. If you needed it to work with less sophisticated engines it
> shouldn't be too hard to dumb down the regex above.
>
> Live-ish example: https://regex101.com/r/wvHLpa/1
>
> Is there a special property that makes single lines much easier to
> grep? Something specific to bash? I wouldn't think bash would have any
> problems looping over this by line.
>
> Thanks,
> Kevin
>
Kevin Mark July 19, 2017, 6:01 a.m.
Hello all,

Just wondering if anyone had any thoughts on the grepability issue,
which I believe is the only potential issue/breaking change with this
patch.

Thanks,
Kevin

On Sun, Jun 11, 2017 at 9:59 PM, Kevin Mark <kmark937@gmail.com> wrote:
> Hi Michael,
>
> Hoping to get your thoughts on the grepability issue (wrt my previous
> email). If it needs to be on a single line there's no reason the new format
> cannot be changed to do so (removing the \n and adding a separator, really).
> However I'm a big fan of it as-is (for both scale and scale2ref) and I can't
> find a case of my own where the regex I proposed would be troublesome.
>
> Thanks,
> Kevin
>
> On Tue, Jun 6, 2017 at 4:17 PM Kevin Mark <kmark937@gmail.com> wrote:
>>
>> On Tue, Jun 6, 2017 at 11:49 AM, Michael Niedermayer
>> <michael@niedermayer.cc> wrote:
>> > yes but its much harder to grep for as its not a single line anymore
>>
>> I agree that it's not going to be as pretty a regular expression to
>> grep through, as there is 33% more data, but it should still be doable
>> without too much effort. How important is it that we maintain "API"
>> compatibility on verbose CLI output?
>>
>> ffmpeg [...] scale2ref=0:0 [...] -v verbose - 2>&1 >/dev/null | grep -oP
>> 'regex'
>>
>> Where regex is:
>>
>> (in|out|ref) +w:(\d+) h:(\d+) fmt:(\w+) sar:(\d+)\/(\d+)(?:
>> flags:0x[[:xdigit:]]+)?
>>
>> Assuming GNU grep 2.25+, you'll get:
>>
>> in  w:320 h:240 fmt:rgb24 sar:1/1
>> ref w:640 h:360 fmt:rgb24 sar:1/1
>> out w:640 h:360 fmt:rgb24 sar:3/4 flags:0x2
>>
>> It also works with BSD grep 2.5.1-FreeBSD included in macOS if you use
>> the -E option instead of -P. These would be considered three separate
>> matches so if you're using a good regex engine it'd be pretty easy to
>> loop over each match, check the first group to determine if it's in,
>> ref, or out and act accordingly on the rest of the captured data. You
>> could also, if you wanted, assume that the first line is in and the
>> second line is out if you only have two matches (or lines even) and if
>> you have three matches/lines the first is in, second is ref, third is
>> out. If you needed it to work with less sophisticated engines it
>> shouldn't be too hard to dumb down the regex above.
>>
>> Live-ish example: https://regex101.com/r/wvHLpa/1
>>
>> Is there a special property that makes single lines much easier to
>> grep? Something specific to bash? I wouldn't think bash would have any
>> problems looping over this by line.
>>
>> Thanks,
>> Kevin
Ronald S. Bultje July 19, 2017, 10:44 a.m.
Hi Kevin,

On Wed, Jul 19, 2017 at 2:01 AM, Kevin Mark <kmark937@gmail.com> wrote:

> Hello all,
>
> Just wondering if anyone had any thoughts on the grepability issue,
> which I believe is the only potential issue/breaking change with this
> patch.


You wrote something like:

"ffmpeg [...] scale2ref=0:0 [...] -v verbose - 2>&1 >/dev/null | grep -oP
'regex'

Where regex is: [..]"

But my grep (Mac) has no -P option... I'd encourage you to keep things
simple and use a non-\n delimiter between the lines so non-GNU grep can
continue to be used for this also...

Ronald
Kevin Mark July 24, 2017, 10:27 a.m.
Hi Ronald,

On Wed, Jul 19, 2017 at 3:44 AM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> But my grep (Mac) has no -P option... I'd encourage you to keep things
> simple and use a non-\n delimiter between the lines so non-GNU grep can
> continue to be used for this also...

Does this work for you:

On Tue, Jun 6, 2017 at 1:17 PM, Kevin Mark <kmark937@gmail.com> wrote:
> It also works with BSD grep 2.5.1-FreeBSD included in macOS if you use
> the -E option instead of -P.

Thanks,
Kevin
Kevin Mark Sept. 23, 2017, 6:22 a.m.
Hey Ronald,

Was this able to solve the issue for you?

On Mon, Jul 24, 2017 at 6:27 AM, Kevin Mark <kmark937@gmail.com> wrote:
> Hi Ronald,
>
> On Wed, Jul 19, 2017 at 3:44 AM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
>> But my grep (Mac) has no -P option... I'd encourage you to keep things
>> simple and use a non-\n delimiter between the lines so non-GNU grep can
>> continue to be used for this also...
>
> Does this work for you:
>
> On Tue, Jun 6, 2017 at 1:17 PM, Kevin Mark <kmark937@gmail.com> wrote:
>> It also works with BSD grep 2.5.1-FreeBSD included in macOS if you use
>> the -E option instead of -P.
>
> Thanks,
> Kevin
Ronald S. Bultje Sept. 23, 2017, 3:54 p.m.
Hi Kevin,

On Sat, Sep 23, 2017 at 2:22 AM, Kevin Mark <kmark937@gmail.com> wrote:

> Hey Ronald,
>
> Was this able to solve the issue for you?
>
> On Mon, Jul 24, 2017 at 6:27 AM, Kevin Mark <kmark937@gmail.com> wrote:
> > Hi Ronald,
> >
> > On Wed, Jul 19, 2017 at 3:44 AM, Ronald S. Bultje <rsbultje@gmail.com>
> wrote:
> >> But my grep (Mac) has no -P option... I'd encourage you to keep things
> >> simple and use a non-\n delimiter between the lines so non-GNU grep can
> >> continue to be used for this also...
> >
> > Does this work for you:
> >
> > On Tue, Jun 6, 2017 at 1:17 PM, Kevin Mark <kmark937@gmail.com> wrote:
> >> It also works with BSD grep 2.5.1-FreeBSD included in macOS if you use
> >> the -E option instead of -P.
>

Yes, -E works on Mac.

Ronald
Kevin Mark Sept. 25, 2017, 1:14 a.m.
Hi Ronald,

On Sat, Sep 23, 2017 at 11:54 AM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
>
> Yes, -E works on Mac.

Thanks! Are there any remaining blockers for a potential merge?

Best regards,
Kevin
Michael Niedermayer Sept. 25, 2017, 8:41 p.m.
On Sun, Sep 24, 2017 at 09:14:28PM -0400, Kevin Mark wrote:
> Hi Ronald,
> 
> On Sat, Sep 23, 2017 at 11:54 AM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> >
> > Yes, -E works on Mac.
> 
> Thanks! Are there any remaining blockers for a potential merge?

There is no gurantee that 3 av_log() calls will be executed with no
other log calls from other threads between.
A multiline grep is not reliable in this case

[...]

Patch hide | download patch | download mbox

diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
index c59ac6b0ea..9232bc4439 100644
--- a/libavfilter/vf_scale.c
+++ b/libavfilter/vf_scale.c
@@ -342,9 +342,18 @@  static int config_props(AVFilterLink *outlink)
     } else
         outlink->sample_aspect_ratio = inlink->sample_aspect_ratio;
 
-    av_log(ctx, AV_LOG_VERBOSE, "w:%d h:%d fmt:%s sar:%d/%d -> w:%d h:%d fmt:%s sar:%d/%d flags:0x%0x\n",
-           inlink ->w, inlink ->h, av_get_pix_fmt_name( inlink->format),
-           inlink->sample_aspect_ratio.num, inlink->sample_aspect_ratio.den,
+    if (ctx->filter == &ff_vf_scale2ref) {
+        av_log(ctx, AV_LOG_VERBOSE, "in  w:%d h:%d fmt:%s sar:%d/%d\n",
+               inlink0->w, inlink0->h, av_get_pix_fmt_name(inlink0->format),
+               inlink0->sample_aspect_ratio.num, inlink0->sample_aspect_ratio.den);
+    }
+
+    av_log(ctx, AV_LOG_VERBOSE, "%s w:%d h:%d fmt:%s sar:%d/%d\n",
+           ctx->filter == &ff_vf_scale2ref ? "ref" : "in ",
+           inlink->w, inlink->h, av_get_pix_fmt_name(inlink->format),
+           inlink->sample_aspect_ratio.num, inlink->sample_aspect_ratio.den);
+
+    av_log(ctx, AV_LOG_VERBOSE, "out w:%d h:%d fmt:%s sar:%d/%d flags:0x%0x\n",
            outlink->w, outlink->h, av_get_pix_fmt_name(outlink->format),
            outlink->sample_aspect_ratio.num, outlink->sample_aspect_ratio.den,
            scale->flags);