On Thu, Sep 17, 2020 at 11:44:39PM +0300, Jan Ekström wrote: > On Thu, Sep 17, 2020 at 11:31 PM Michael Niedermayer > <mich...@niedermayer.cc> wrote: > > > > On Wed, Sep 16, 2020 at 11:18:48PM +0300, Jan Ekström wrote: > > > This value - while it looks like the actual range of the content - > > > is nothing but the internal value of swscale. > > > > > > Thus, if we have RGB content, force the value to 1. Swscale will > > > ignore it, but at least the value of the output AVFrame will now > > > properly be "full range" instead of "limited range", as it is right > > > now. > > > > > > Additionally, after calling sws_setColorspaceDetails double-check > > > the configured internal flag for the color range. Warn if this is > > > different to the requested value. > > > > > > Finally, utilize the translated configured output value into the > > > output AVFrame's color_range. > > > --- > > > libavfilter/vf_scale.c | 51 +++++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 50 insertions(+), 1 deletion(-) > > > > > > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c > > > index 58eee96744..592e4a344e 100644 > > > --- a/libavfilter/vf_scale.c > > > +++ b/libavfilter/vf_scale.c > > > @@ -750,11 +750,30 @@ scale: > > > || in_range != AVCOL_RANGE_UNSPECIFIED > > > || scale->out_range != AVCOL_RANGE_UNSPECIFIED) { > > > int in_full, out_full, brightness, contrast, saturation; > > > + int configured_in_full_range_flag, > > > configured_out_full_range_flag; > > > const int *inv_table, *table; > > > + const AVPixFmtDescriptor *in_desc = > > > av_pix_fmt_desc_get(in->format); > > > + const AVPixFmtDescriptor *out_desc = > > > av_pix_fmt_desc_get(out->format); > > > > > + if (!in_desc || !out_desc) { > > > + av_log(ctx, AV_LOG_ERROR, > > > + "Failed to get one or more of the pixel format > > > descriptors " > > > + "for formats - in: %d (%s), out: %d (%s)!\n", > > > + in->format, in_desc ? "OK" : "bad", > > > + out->format, out_desc ? "OK": "bad"); > > > + av_frame_free(&in); > > > + av_frame_free(frame_out); > > > + return AVERROR_INVALIDDATA; > > > + } > > > > can this be true ? > > > > Maybe it cannot, but it's a pointer. If the value somehow ends up > being one for which there is no descriptor. > > It all boils to, unless I know there cannot technically be a nullptr > received, I check. Because this way the check is in one place, instead > of doing `xx_desc ? xx_desc->thing : failover` everywhere. >
> I can change this to an assert as mentioned by Nicolas, of course. please do if theres a NULL check, both the human reader as well as things like coverity assume it can be NULL. And both get confused if thats not actually possible > > > > > > > > > sws_getColorspaceDetails(scale->sws, (int **)&inv_table, > > > &in_full, > > > (int **)&table, &out_full, > > > &brightness, &contrast, &saturation); > > > > > + // translate the swscale internal range flags to hold true > > > + // for RGB > > > > The range field of AVFrame is documented as > > "MPEG vs JPEG YUV range." > > > > Yes, but I think quite a few people at this point just read it "full > range" VS "limited range" without any connotations towards YCbCr > specifically. Personally, I consider the enumeration names as just old > left-overs from the time when the MPEG/JPEG or TV/PC naming sounded > like a good idea and that nobody wanted to start the change of adding > new defines and enum values etc, doing deprecation etc as that is > quite a lot of work just to get the naming of enums nicer. Case in > point - for example pngdec sets the range value for the RGB frames it > returns. > > Of course unless you specifically want to support limited range RGB, > RGB should not only be implied being full range by default but also > always set to be full range everywhere. > > > so it is not defined for RGB and cannot be "wrong" for RGB > > maybe iam nitpicking here but if you want to use the range for RGB > > the API docs need to be changed first. > > > > The code was ALREADY doing that, and I'm just converting the value so > that you don't end up with RGB + limited range AVFrames?! > > > <super nitpick>I mean you could set it to 1 for RGB thats ok without a API > > docs change. But writing in a comment that one way is correct for RGB is not > > compatible with the current documentation</super nitpick> > > > > So you want another patch that changes it to just say "limited/narrow > range" and "full range"? Like seriously, I just wanted to fix a > problem that got found because I finally started plugging some pipes > together! If you want to change the range enum and field so it also applies to RGB and not just YUV. That should be done in a consistent and complete way and i imagine will not be just "one more thing" otherwise if the API isnt changed, this variable is not describing RGB I dont think theres a problem if you set it to a specific value for RGB in that case but you cannot call it a bug or wrong if its not set that way. Basically code which expects it to be a specific value for RGB is where the bug is. also the full YUV range is 2 and limited range is 1. 0 is unspecified in our enum. so i wonder if RGB shouldnt be AVCOL_RANGE_UNSPECIFIED instead of AVCOL_RANGE_JPEG which i think is what this patch would do. That is if the range API isnt extended to include RGB. All that said, i think extending the range API to cover RGB would be a good idea. But its surely more work thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Concerning the gods, I have no means of knowing whether they exist or not or of what sort they may be, because of the obscurity of the subject, and the brevity of human life -- Protagoras
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".