| Message ID | 20170605105521.45420-1-kmark937@gmail.com |
|---|---|
| State | New |
| Headers | show |
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 [...]
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
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 >
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
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
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
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
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
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
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 [...]
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);
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(-)