Re: [FFmpeg-devel] [PATCH] avcodec/tiff: Check for Tiled and Stripped TIFFs
On Thu, May 21, 2020 at 12:13:15PM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2020-05-21 08:59:27) > > > > I dont see IS_IRAP_NAL() and IS_IDR_NAL() to check every field in the > > headers > > to be consistent with IDR / IRAP NALs, which is what the tiff code in this > > thread does kind off .. > > > > to match hevc, i could add something like > > #define HAVE_TILES(s) s->has_tiles > > #define HAVE_STRIPS(s) s->strips > > > > but iam not sure what that would simplify > > The validity checks are needed once and if thats put in a macro then maybe > > that > > could be called HAVE_RANDOM_TILE_BITS() and HAVE_RANDOM_STRIP_BITS() could > > be > > maybe a possibility > > then again if thats what is suggested then i would suggest to rather add 2 > > local variable with these names instead of a macro which would seem more > > readable > > > > whats your opinion ? > > I see little difference between a macro and a local variable in this > case, either one is fine with me. And both are significantly more > readable than a giant condition that the reader has to reverse engineer > into (have_tiles && have_strips) ok, will apply it with local variables thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In fact, the RIAA has been known to suggest that students drop out of college or go to community college in order to be able to afford settlements. -- The RIAA 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".
Re: [FFmpeg-devel] [PATCH] avcodec/tiff: Check for Tiled and Stripped TIFFs
Quoting Michael Niedermayer (2020-05-21 08:59:27) > > I dont see IS_IRAP_NAL() and IS_IDR_NAL() to check every field in the headers > to be consistent with IDR / IRAP NALs, which is what the tiff code in this > thread does kind off .. > > to match hevc, i could add something like > #define HAVE_TILES(s) s->has_tiles > #define HAVE_STRIPS(s) s->strips > > but iam not sure what that would simplify > The validity checks are needed once and if thats put in a macro then maybe > that > could be called HAVE_RANDOM_TILE_BITS() and HAVE_RANDOM_STRIP_BITS() could be > maybe a possibility > then again if thats what is suggested then i would suggest to rather add 2 > local variable with these names instead of a macro which would seem more > readable > > whats your opinion ? I see little difference between a macro and a local variable in this case, either one is fine with me. And both are significantly more readable than a giant condition that the reader has to reverse engineer into (have_tiles && have_strips) -- Anton Khirnov ___ 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".
Re: [FFmpeg-devel] [PATCH] avcodec/tiff: Check for Tiled and Stripped TIFFs
On Thu, Feb 27, 2020 at 12:07:16PM -0300, James Almer wrote:
> On 2/27/2020 11:10 AM, Carl Eugen Hoyos wrote:
> > Am Do., 27. Feb. 2020 um 15:05 Uhr schrieb Anton Khirnov
> > :
> >>
> >> Quoting Michael Niedermayer (2020-02-19 16:49:51)
> >>> TIFF 6 spec: "Do not use both strip-oriented and tile-oriented fields in
> >>> the same TIFF file."
> >>>
> >>> Fixes: null pointer use, crash
> >>> Fixes: crash-762680f9d1b27f9b9085e12887ad44893fb2b020
> >>>
> >>> Found-by: Shiziru
> >>> Signed-off-by: Michael Niedermayer
> >>> ---
> >>> libavcodec/tiff.c | 6 ++
> >>> 1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> >>> index e8357114de..fd50b1cbfa 100644
> >>> --- a/libavcodec/tiff.c
> >>> +++ b/libavcodec/tiff.c
> >>> @@ -1873,6 +1873,12 @@ again:
> >>> return AVERROR_INVALIDDATA;
> >>> }
> >>>
> >>> +if ( (s->is_tiled || s->tile_byte_counts_offset ||
> >>> s->tile_offsets_offset || s->tile_width || s->tile_length ||
> >>> s->tile_count)
> >>> +&& (s->strippos || s->strips || s->stripoff || s->rps || s->sot
> >>> || s->sstype || s->stripsize || s->stripsizesoff)) {
> >>
> >> This is horribly unreadable. Putting those checks into macros, like
> >> HAVE_TILES(s) and HAVE_STRIPS(s) would make it a lot better.
> >
> > Were else in the file could the macros be used?
> > I fear that adding macros that are only used in one place will make the
> > code much more unreadable.
>
> I don't agree. An "if (HAVE_TILES(s) && HAVE_STRIPS(s))" check is easy
> to understand at first glance, regardless of the internals of each of
> those macros. It achieves the same effect as adding a comment in the
> code to explain what all those checks do.
>
> See IS_IRAP_NAL(nal) and IS_IDR_NAL(nal) in hevc_parser for another
> example of this simplification, which are also used in a single place.
I dont see IS_IRAP_NAL() and IS_IDR_NAL() to check every field in the headers
to be consistent with IDR / IRAP NALs, which is what the tiff code in this
thread does kind off ..
to match hevc, i could add something like
#define HAVE_TILES(s) s->has_tiles
#define HAVE_STRIPS(s) s->strips
but iam not sure what that would simplify
The validity checks are needed once and if thats put in a macro then maybe that
could be called HAVE_RANDOM_TILE_BITS() and HAVE_RANDOM_STRIP_BITS() could be
maybe a possibility
then again if thats what is suggested then i would suggest to rather add 2
local variable with these names instead of a macro which would seem more
readable
whats your opinion ?
Thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato
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".
Re: [FFmpeg-devel] [PATCH] avcodec/tiff: Check for Tiled and Stripped TIFFs
On 2/27/2020 11:10 AM, Carl Eugen Hoyos wrote:
> Am Do., 27. Feb. 2020 um 15:05 Uhr schrieb Anton Khirnov :
>>
>> Quoting Michael Niedermayer (2020-02-19 16:49:51)
>>> TIFF 6 spec: "Do not use both strip-oriented and tile-oriented fields in
>>> the same TIFF file."
>>>
>>> Fixes: null pointer use, crash
>>> Fixes: crash-762680f9d1b27f9b9085e12887ad44893fb2b020
>>>
>>> Found-by: Shiziru
>>> Signed-off-by: Michael Niedermayer
>>> ---
>>> libavcodec/tiff.c | 6 ++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
>>> index e8357114de..fd50b1cbfa 100644
>>> --- a/libavcodec/tiff.c
>>> +++ b/libavcodec/tiff.c
>>> @@ -1873,6 +1873,12 @@ again:
>>> return AVERROR_INVALIDDATA;
>>> }
>>>
>>> +if ( (s->is_tiled || s->tile_byte_counts_offset ||
>>> s->tile_offsets_offset || s->tile_width || s->tile_length || s->tile_count)
>>> +&& (s->strippos || s->strips || s->stripoff || s->rps || s->sot ||
>>> s->sstype || s->stripsize || s->stripsizesoff)) {
>>
>> This is horribly unreadable. Putting those checks into macros, like
>> HAVE_TILES(s) and HAVE_STRIPS(s) would make it a lot better.
>
> Were else in the file could the macros be used?
> I fear that adding macros that are only used in one place will make the
> code much more unreadable.
I don't agree. An "if (HAVE_TILES(s) && HAVE_STRIPS(s))" check is easy
to understand at first glance, regardless of the internals of each of
those macros. It achieves the same effect as adding a comment in the
code to explain what all those checks do.
See IS_IRAP_NAL(nal) and IS_IDR_NAL(nal) in hevc_parser for another
example of this simplification, which are also used in a single place.
>
> Carl Eugen
> ___
> 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".
>
___
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".
Re: [FFmpeg-devel] [PATCH] avcodec/tiff: Check for Tiled and Stripped TIFFs
Am Do., 27. Feb. 2020 um 15:05 Uhr schrieb Anton Khirnov :
>
> Quoting Michael Niedermayer (2020-02-19 16:49:51)
> > TIFF 6 spec: "Do not use both strip-oriented and tile-oriented fields in
> > the same TIFF file."
> >
> > Fixes: null pointer use, crash
> > Fixes: crash-762680f9d1b27f9b9085e12887ad44893fb2b020
> >
> > Found-by: Shiziru
> > Signed-off-by: Michael Niedermayer
> > ---
> > libavcodec/tiff.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> > index e8357114de..fd50b1cbfa 100644
> > --- a/libavcodec/tiff.c
> > +++ b/libavcodec/tiff.c
> > @@ -1873,6 +1873,12 @@ again:
> > return AVERROR_INVALIDDATA;
> > }
> >
> > +if ( (s->is_tiled || s->tile_byte_counts_offset ||
> > s->tile_offsets_offset || s->tile_width || s->tile_length || s->tile_count)
> > +&& (s->strippos || s->strips || s->stripoff || s->rps || s->sot ||
> > s->sstype || s->stripsize || s->stripsizesoff)) {
>
> This is horribly unreadable. Putting those checks into macros, like
> HAVE_TILES(s) and HAVE_STRIPS(s) would make it a lot better.
Were else in the file could the macros be used?
I fear that adding macros that are only used in one place will make the
code much more unreadable.
Carl Eugen
___
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".
Re: [FFmpeg-devel] [PATCH] avcodec/tiff: Check for Tiled and Stripped TIFFs
Quoting Michael Niedermayer (2020-02-19 16:49:51)
> TIFF 6 spec: "Do not use both strip-oriented and tile-oriented fields in the
> same TIFF file."
>
> Fixes: null pointer use, crash
> Fixes: crash-762680f9d1b27f9b9085e12887ad44893fb2b020
>
> Found-by: Shiziru
> Signed-off-by: Michael Niedermayer
> ---
> libavcodec/tiff.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> index e8357114de..fd50b1cbfa 100644
> --- a/libavcodec/tiff.c
> +++ b/libavcodec/tiff.c
> @@ -1873,6 +1873,12 @@ again:
> return AVERROR_INVALIDDATA;
> }
>
> +if ( (s->is_tiled || s->tile_byte_counts_offset ||
> s->tile_offsets_offset || s->tile_width || s->tile_length || s->tile_count)
> +&& (s->strippos || s->strips || s->stripoff || s->rps || s->sot ||
> s->sstype || s->stripsize || s->stripsizesoff)) {
This is horribly unreadable. Putting those checks into macros, like
HAVE_TILES(s) and HAVE_STRIPS(s) would make it a lot better.
--
Anton Khirnov
___
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".
[FFmpeg-devel] [PATCH] avcodec/tiff: Check for Tiled and Stripped TIFFs
TIFF 6 spec: "Do not use both strip-oriented and tile-oriented fields in the
same TIFF file."
Fixes: null pointer use, crash
Fixes: crash-762680f9d1b27f9b9085e12887ad44893fb2b020
Found-by: Shiziru
Signed-off-by: Michael Niedermayer
---
libavcodec/tiff.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
index e8357114de..fd50b1cbfa 100644
--- a/libavcodec/tiff.c
+++ b/libavcodec/tiff.c
@@ -1873,6 +1873,12 @@ again:
return AVERROR_INVALIDDATA;
}
+if ( (s->is_tiled || s->tile_byte_counts_offset ||
s->tile_offsets_offset || s->tile_width || s->tile_length || s->tile_count)
+&& (s->strippos || s->strips || s->stripoff || s->rps || s->sot ||
s->sstype || s->stripsize || s->stripsizesoff)) {
+av_log(avctx, AV_LOG_ERROR, "Tiled TIFF is not allowed to strip\n");
+return AVERROR_INVALIDDATA;
+}
+
/* now we have the data and may start decoding */
if ((ret = init_image(s, )) < 0)
return ret;
--
2.17.1
___
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".
