| Message ID | 20170527141046.57488-1-kmark937@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, May 27, 2017 at 7:40 PM, Kevin Mark <kmark937@gmail.com> wrote: > Variables pertaining to the non-reference video are now available when > using the scale2ref filter. This allows scaling a video with another > as a reference point while maintaining the original aspect ratio of > the non-reference video. > Once applied, leave a note at (and close) https://trac.ffmpeg.org/ticket/5392
Thank you, I was not aware there was a ticket already for this issue. I wouldn't be comfortable calling this a bug fix (or that ticket being a bug/defect) considering the current functionality, while not particularly intuitive, is consistent with the documentation. Patch 3 helps clarify the scale2ref documentation a tad but there's still more room to clarify the wording, no doubt. It's not explicit that "as basis" means all the scale variables will be set to that of the reference input. I'll consider submitting another patch for that after this is applied/discussed. On Sat, May 27, 2017 at 10:49 AM, Gyan <gyandoshi@gmail.com> wrote: > On Sat, May 27, 2017 at 7:40 PM, Kevin Mark <kmark937@gmail.com> wrote: > > > Variables pertaining to the non-reference video are now available when > > using the scale2ref filter. This allows scaling a video with another > > as a reference point while maintaining the original aspect ratio of > > the non-reference video. > > > > Once applied, leave a note at (and close) > https://trac.ffmpeg.org/ticket/5392 > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Sat, May 27, 2017 at 9:41 PM, Kevin Mark <kmark937@gmail.com> wrote: > I wouldn't be comfortable calling this a bug fix (or that ticket being a > bug/defect) considering the current functionality, while not particularly > intuitive, is consistent with the documentation. > The commit message for this filter says, "This filter can be used to scale one stream to match another or based on another, useful to scale subtitles or other things to be overlayed" It's weird, given those uses cases - overlays - to override the operand's native aspect ratio. I'd say, a design oversight, at least.
On Sat, May 27, 2017 at 10:10:44AM -0400, Kevin Mark wrote: > Variables pertaining to the non-reference video are now available when > using the scale2ref filter. This allows scaling a video with another > as a reference point while maintaining the original aspect ratio of > the non-reference video. > > Consider the following graph: scale2ref=iw/6:-1 [scaled][ref] > This will scale [scaled] to 1/6 the width of [ref] while maintaining > the aspect ratio. This works well when the AR of [ref] is equal to > the AR of [scaled] only. What the above filter really does is > maintain the AR of [ref] when scaling [scaled]. So in all non-same-AR > situations [scaled] will appear stretched or compressed to conform to > the same AR of the reference video. Without doing this calculation > externally there is no way to scale in reference to another input > while maintaining AR in libavfilter. > > To make this possible, we introduce seven new constants to be used > in the w and h expressions only in the scale2ref filter: > > * other_w/other_h: width/height of the input video being scaled > * other_a: aspect ratio of the input video being scaled > * other_sar: sample aspect ratio of the input video being scaled > * other_dar: display aspect ratio of the input video being scaled > * other_hsub/other_vsub: horiz/vert chroma subsample vals of input > > Of course the only new constants needed for maintaining the AR is > other_w/other_h or other_a but adding additional constants in line of > what is available for in/out allows for other scaling possibilities I > have not imagined. > > So to now scale a video to 1/6 the size of another video using the > width and maintaining its own aspect ratio you can do this: > > scale2ref=iw/6:iw/(6*other_a) [scaled][ref] > > This is ideal for picture-in-picture configurations where you could > have a square or 4:3 video overlaid on a corner of a larger 16:9 > feed all while keeping the scaled video in the corner at its correct > aspect ratio and always the same size relative to the larger video. > > I am not fully sold on the other_ prefix. It works but I'm open to > suggestions on something that is more concise or clear. Unfortunately > in_ is most clear but that is taken by the reference input, which in > my opinion, should've been labeled as such (not as in_ but perhaps > ref_). What is the best way to refer to this? The other input? The > scaled input? The non-reference input? main_* and ref_* are maybe prefixes that would work without conflicting with the existing ones [...]
On Sat, May 27, 2017 at 1:36 PM, Gyan <gyandoshi@gmail.com> wrote: > It's weird, given those uses cases - overlays - to override the operand's > native aspect ratio. I'd say, a design oversight, at least. I agree. I'm not aware of any common workflow that you would have subtitles (transparent PNGs or something) that would be the same AR as the main video but not the same size (as evidence by the fact that scale2ref is needed at all). Oh well. These additional constants/variables should tackle that ticket without needing to introduce overly specific special cases like the suggested keep-AR flag.
On Sat, May 27, 2017 at 8:29 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > main_* > and > ref_* > > are maybe prefixes that would work without conflicting with the > existing ones I'm partial to main_* since when I think of ref_* that makes me think of the input being used "as basis" (in the words of the documentation) or reference. Any thoughts on creating shorthand variables? I don't see a particular need for them but it would put these new variables in line with the others. mw for main_w, mh for main_h. Potentially ma for main_a. I don't have a strong feeling on it one way or another. This is my first patch set to FFmpeg (huzzah!) so I have a few quick questions before continuing. What is the preferred way to revise a patch? From my understanding git send-email is only useful for the initial patch. Unless revised patches should be new threads? Or should I just reply to this thread with attachments from git format-patch? Although now that I'm looking at it, git send-email has an --in-reply-to option that could potentially do the trick of submitting the revised patch. This mailing list patch submission workflow is new to me so I appreciate any guidance. Thanks, Kevin
On Sun, May 28, 2017 at 8:06 PM, Kevin Mark <kmark937@gmail.com> wrote: > Any thoughts on creating shorthand variables? I don't > see a particular need for them but it would put these new variables in > line with the others. mw for main_w, mh for main_h. Potentially ma for > main_a. I don't have a strong feeling on it one way or another. > If you do, I suggest 'mdar' for main_dar which is the only one that ought to be required. The scale filter adjusts the SAR of the result so that the DAR is preserved i.e. when a 100x100 canvas w/ SAR of 2.0 is scaled to 50x100, the result will have SAR 4.0. In the scale2ref filter, this adjustment is made so that the ref's DAR is effected onto the scaled result. I don't believe your patch changes this behaviour. Now, the overlay filter does not respect the overlay input's SAR - a 100x100 canvas of SAR 2 is overlaid as a square - so having a shorthand for the main DAR and promoting its use will silently enforce a visually correct-looking input for overlays and other compositing uses.
On Sun, May 28, 2017 at 12:14 PM, Gyan <gyandoshi@gmail.com> wrote: > If you do, I suggest 'mdar' for main_dar which is the only one that ought > to be required. > > The scale filter adjusts the SAR of the result so that the DAR is preserved > i.e. when a 100x100 canvas w/ SAR of 2.0 is scaled to 50x100, the result > will have SAR 4.0. In the scale2ref filter, this adjustment is made so that > the ref's DAR is effected onto the scaled result. I don't believe your > patch changes this behaviour. Now, the overlay filter does not respect the > overlay input's SAR - a 100x100 canvas of SAR 2 is overlaid as a square - > so having a shorthand for the main DAR and promoting its use will silently > enforce a visually correct-looking input for overlays and other compositing > uses. Sounds good. If my understanding is correct, you would recommend changing the scale2ref=iw/6:iw/(6*main_a) [main][ref] example to scale2ref=iw/6:iw/(6*mdar) [main][ref] correct? Thanks, Kevin
On Tue, May 30, 2017 at 3:27 PM, Kevin Mark <kmark937@gmail.com> wrote: > > Sounds good. If my understanding is correct, you would recommend changing > the > scale2ref=iw/6:iw/(6*main_a) [main][ref] > example to > scale2ref=iw/6:iw/(6*mdar) [main][ref] > correct? > I prefer scale2ref=iw/6:ow/mdar [main][ref] or scale2ref=oh*mdar:ih/6 [main][ref] This way, the expression complementary to the one specified remains fixed.
diff --git a/libavfilter/scale.c b/libavfilter/scale.c index 50cd442849..9b4846719d 100644 --- a/libavfilter/scale.c +++ b/libavfilter/scale.c @@ -60,6 +60,49 @@ enum var_name { VARS_NB }; +/** + * This must be kept in sync with var_names so that it is always a + * complete list of var_names with the scale2ref specific names + * appended. scale2ref values must appear in the order they appear + * in the var_name_scale2ref enum but also be below all of the + * non-scale2ref specific values. + */ +static const char *const var_names_scale2ref[] = { + "PI", + "PHI", + "E", + "in_w", "iw", + "in_h", "ih", + "out_w", "ow", + "out_h", "oh", + "a", + "sar", + "dar", + "hsub", + "vsub", + "ohsub", + "ovsub", + "other_w", + "other_h", + "other_a", + "other_sar", + "other_dar", + "other_hsub", + "other_vsub", + NULL +}; + +enum var_name_scale2ref { + VAR_S2R_OTHER_W, + VAR_S2R_OTHER_H, + VAR_S2R_OTHER_A, + VAR_S2R_OTHER_SAR, + VAR_S2R_OTHER_DAR, + VAR_S2R_OTHER_HSUB, + VAR_S2R_OTHER_VSUB, + VARS_S2R_NB +}; + int ff_scale_eval_dimensions(void *log_ctx, const char *w_expr, const char *h_expr, AVFilterLink *inlink, AVFilterLink *outlink, @@ -72,7 +115,16 @@ int ff_scale_eval_dimensions(void *log_ctx, int factor_w, factor_h; int eval_w, eval_h; int ret; - double var_values[VARS_NB], res; + const char scale2ref = outlink->src->inputs[1] == inlink; + double var_values[VARS_NB + VARS_S2R_NB], res; + const AVPixFmtDescriptor *other_desc; + const AVFilterLink *other; + const char *const *names = scale2ref ? var_names_scale2ref : var_names; + + if (scale2ref) { + other = outlink->src->inputs[0]; + other_desc = av_pix_fmt_desc_get(other->format); + } var_values[VAR_PI] = M_PI; var_values[VAR_PHI] = M_PHI; @@ -90,20 +142,32 @@ int ff_scale_eval_dimensions(void *log_ctx, var_values[VAR_OHSUB] = 1 << out_desc->log2_chroma_w; var_values[VAR_OVSUB] = 1 << out_desc->log2_chroma_h; + if (scale2ref) { + var_values[VARS_NB + VAR_S2R_OTHER_W] = other->w; + var_values[VARS_NB + VAR_S2R_OTHER_H] = other->h; + var_values[VARS_NB + VAR_S2R_OTHER_A] = (double) other->w / other->h; + var_values[VARS_NB + VAR_S2R_OTHER_SAR] = other->sample_aspect_ratio.num ? + (double) other->sample_aspect_ratio.num / other->sample_aspect_ratio.den : 1; + var_values[VARS_NB + VAR_S2R_OTHER_DAR] = var_values[VARS_NB + VAR_S2R_OTHER_A] * + var_values[VARS_NB + VAR_S2R_OTHER_SAR]; + var_values[VARS_NB + VAR_S2R_OTHER_HSUB] = 1 << other_desc->log2_chroma_w; + var_values[VARS_NB + VAR_S2R_OTHER_VSUB] = 1 << other_desc->log2_chroma_h; + } + /* evaluate width and height */ av_expr_parse_and_eval(&res, (expr = w_expr), - var_names, var_values, + names, var_values, NULL, NULL, NULL, NULL, NULL, 0, log_ctx); eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; if ((ret = av_expr_parse_and_eval(&res, (expr = h_expr), - var_names, var_values, + names, var_values, NULL, NULL, NULL, NULL, NULL, 0, log_ctx)) < 0) goto fail; eval_h = var_values[VAR_OUT_H] = var_values[VAR_OH] = res; /* evaluate again the width, as it may depend on the output height */ if ((ret = av_expr_parse_and_eval(&res, (expr = w_expr), - var_names, var_values, + names, var_values, NULL, NULL, NULL, NULL, NULL, 0, log_ctx)) < 0) goto fail; eval_w = res;
Variables pertaining to the non-reference video are now available when using the scale2ref filter. This allows scaling a video with another as a reference point while maintaining the original aspect ratio of the non-reference video. Consider the following graph: scale2ref=iw/6:-1 [scaled][ref] This will scale [scaled] to 1/6 the width of [ref] while maintaining the aspect ratio. This works well when the AR of [ref] is equal to the AR of [scaled] only. What the above filter really does is maintain the AR of [ref] when scaling [scaled]. So in all non-same-AR situations [scaled] will appear stretched or compressed to conform to the same AR of the reference video. Without doing this calculation externally there is no way to scale in reference to another input while maintaining AR in libavfilter. To make this possible, we introduce seven new constants to be used in the w and h expressions only in the scale2ref filter: * other_w/other_h: width/height of the input video being scaled * other_a: aspect ratio of the input video being scaled * other_sar: sample aspect ratio of the input video being scaled * other_dar: display aspect ratio of the input video being scaled * other_hsub/other_vsub: horiz/vert chroma subsample vals of input Of course the only new constants needed for maintaining the AR is other_w/other_h or other_a but adding additional constants in line of what is available for in/out allows for other scaling possibilities I have not imagined. So to now scale a video to 1/6 the size of another video using the width and maintaining its own aspect ratio you can do this: scale2ref=iw/6:iw/(6*other_a) [scaled][ref] This is ideal for picture-in-picture configurations where you could have a square or 4:3 video overlaid on a corner of a larger 16:9 feed all while keeping the scaled video in the corner at its correct aspect ratio and always the same size relative to the larger video. I am not fully sold on the other_ prefix. It works but I'm open to suggestions on something that is more concise or clear. Unfortunately in_ is most clear but that is taken by the reference input, which in my opinion, should've been labeled as such (not as in_ but perhaps ref_). What is the best way to refer to this? The other input? The scaled input? The non-reference input? I've tried to re-use as much code as possible. I could not find a way to avoid duplication of the var_names array. It must now be kept in sync with the other (the normal one and the scale2ref one) for everything to work which does not seem ideal. For every new variable introduced/removed into/from the normal scale filter one must be added/removed to/from the scale2ref version. Suggestions on how to avoid var_names duplication are welcome. var_values has been increased to always be large enough for the additional scale2ref variables. I do not forsee this being a problem as the names variable will always be the correct size. From my understanding of av_expr_parse_and_eval it will stop processing variables when it runs out of names even though there may be additional (potentially uninitialized) entries in the values array. The ideal solution here would be using a variable-length array but that is unsupported in C90. This patch does not remove any functionality and is strictly a feature patch. There are no API changes. Behavior does not change for any previously valid inputs. Signed-off-by: Kevin Mark <kmark937@gmail.com> --- libavfilter/scale.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 4 deletions(-)