[FFmpeg-devel] avcodec/tiff: Try to fix subsampling default

Submitted by Michael Niedermayer on Sept. 26, 2019, 7:06 a.m.

Details

Message ID 20190926070628.22693-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Sept. 26, 2019, 7:06 a.m.
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(-)

Comments

Skakov Pavel Sept. 28, 2019, 4:13 p.m.
> 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.
Michael Niedermayer Oct. 11, 2019, 8:11 p.m.
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

[...]

Patch hide | download patch | download mbox

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)