| Message ID | 20190926070628.22693-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
> This ensures the default ycbcr_subsampling is 2 while also > ensuring the subsampling values are correct for all pixel formats. > This solution while it takes a few lines more code should be more > robust In TIFF specification only CbCr subsampling is allowed. The field is explicitly named "YCbCrSubsampling", so introducing another subsampling variable serves no purpose other than introducing confusion.
On Sat, Sep 28, 2019 at 07:13:59PM +0300, Skakov Pavel wrote: > >This ensures the default ycbcr_subsampling is 2 while also > >ensuring the subsampling values are correct for all pixel formats. > >This solution while it takes a few lines more code should be more > >robust > > In TIFF specification only CbCr subsampling is allowed. The field is explicitly named "YCbCrSubsampling", so introducing another subsampling variable serves no purpose other than introducing confusion. Well, you are technically correct But having subsampling of 2 "accessible" when subsampling is not 2 is asking for out of array accesses. I mean if you set subsampling so it is always correct for the used pixel format then any code can use it and its fine But if subsampling is only valid for one colorspace and 2 otherwise now you have to proof and maintain that condition that no code ever uses the subsampling value unless the colorspace is ycbcr I have no strong oppinon on this but at the least the subsampling field would need to be renamed to ycbcr_subsampling if its invalid in the more general case thx [...]
diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c index 9f24796a88..13a38bd64f 100644 --- a/libavcodec/tiff.c +++ b/libavcodec/tiff.c @@ -74,6 +74,7 @@ typedef struct TiffContext { enum TiffPhotometric photometric; int planar; int subsampling[2]; + int ycbcr_subsampling[2]; int fax_opts; int predictor; int fill_order; @@ -1101,6 +1102,8 @@ static int init_image(TiffContext *s, ThreadFrame *frame) break; case 243: if (s->photometric == TIFF_PHOTOMETRIC_YCBCR) { + s->subsampling[0] = s->ycbcr_subsampling[0]; + s->subsampling[1] = s->ycbcr_subsampling[1]; if (s->subsampling[0] == 1 && s->subsampling[1] == 1) { s->avctx->pix_fmt = AV_PIX_FMT_YUV444P; } else if (s->subsampling[0] == 2 && s->subsampling[1] == 1) { @@ -1511,10 +1514,10 @@ static int tiff_decode_tag(TiffContext *s, AVFrame *frame) return AVERROR_INVALIDDATA; } for (i = 0; i < count; i++) { - s->subsampling[i] = ff_tget(&s->gb, type, s->le); - if (s->subsampling[i] <= 0) { - av_log(s->avctx, AV_LOG_ERROR, "subsampling %d is invalid\n", s->subsampling[i]); - s->subsampling[i] = 1; + s->ycbcr_subsampling[i] = ff_tget(&s->gb, type, s->le); + if (s->ycbcr_subsampling[i] <= 0) { + av_log(s->avctx, AV_LOG_ERROR, "subsampling %d is invalid\n", s->ycbcr_subsampling[i]); + s->ycbcr_subsampling[i] = 1; return AVERROR_INVALIDDATA; } } @@ -2066,6 +2069,8 @@ static av_cold int tiff_init(AVCodecContext *avctx) s->height = 0; s->subsampling[0] = s->subsampling[1] = 1; + s->ycbcr_subsampling[0] = 2; + s->ycbcr_subsampling[1] = 2; s->avctx = avctx; ff_lzw_decode_open(&s->lzw); if (!s->lzw)
This ensures the default ycbcr_subsampling is 2 while also ensuring the subsampling values are correct for all pixel formats. This solution while it takes a few lines more code should be more robust Found-by: Skakov Pavel <pavelsx@gmail.com> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/tiff.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)