diff mbox series

[FFmpeg-devel,24/35] avutil/opt: AVOptionRange gains is_set field.

Message ID 20210607230414.612-25-dcnieho@gmail.com
State Superseded, archived
Headers show
Series avdevice (mostly dshow) enhancements | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Diederick C. Niehorster June 7, 2021, 11:04 p.m. UTC
When querying a range of formats of an avdevice, sometimes for a given format the queried option is not available. This is not an error as the user is asking for a valid capability, it just doesn't always apply to all the matching formats of the device. This is now communicated through a special value (like 0 or -1), but that is a problem (like always with special values). The is_set field alleviates the use of special values. This will be used when implementing the avdevice capabilities API for avdevice/dshow in a later commit

Signed-off-by: Diederick Niehorster <dcnieho@gmail.com>
---
 libavutil/opt.c     | 1 +
 libavutil/opt.h     | 5 +++++
 libavutil/version.h | 2 +-
 3 files changed, 7 insertions(+), 1 deletion(-)

Comments

Valerii Zapodovnikov June 7, 2021, 11:34 p.m. UTC | #1
Actually it is commonly understood all over the world that limited range is
the default when not present. All video in the world except Dolby Vision
profile 5 (if using IPTPQc2) is limited range.
Andreas Rheinhardt June 7, 2021, 11:45 p.m. UTC | #2
Valerii Zapodovnikov:
> Actually it is commonly understood all over the world that limited range is
> the default when not present. All video in the world except Dolby Vision
> profile 5 (if using IPTPQc2) is limited range.

This has absolutely nothing to do with full/limited range, but rather
whether the AVOptionRange contains an interval or a single value.
(Not that I know why this needs to be set explicitly and not implicitly
via is_range = value_min < value_max.)

- Andreas
Valerii Zapodovnikov June 7, 2021, 11:57 p.m. UTC | #3
Ah, yes, that is AVColorRange, sorry. :( Haha.
Diederick C. Niehorster June 8, 2021, 6:42 a.m. UTC | #4
On Tue, Jun 8, 2021 at 1:45 AM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> This has absolutely nothing to do with full/limited range, but rather
> whether the AVOptionRange contains an interval or a single value.
> (Not that I know why this needs to be set explicitly and not implicitly
> via is_range = value_min < value_max.)
>

Yes, the is_range field seems superfluous, but its already there. What are
your thoughts on the is_set field? For usage example, see patch
device_get_capabilities.c in patch 32/35 in this series.
Diederick C. Niehorster June 11, 2021, 4:46 p.m. UTC | #5
On Tue, Jun 8, 2021 at 1:45 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
> This has absolutely nothing to do with full/limited range, but rather
> whether the AVOptionRange contains an interval or a single value.
> (Not that I know why this needs to be set explicitly and not implicitly
> via is_range = value_min < value_max.)

Hmm, instead of adding a new field, would a bit of documentation be a
good idea? There are three cases:
1. output is a range. Then value_min < value_max.
2. output is set but not a range (single value): Then
value_min==value_max. (We probably shouldn't remove the is_range flag?
OTOH, it is not used in the ffmpeg code base at all...)
3. output is not set: then value_min>value_max (and perhaps special
values value_min=0, value_max=-1. A pair of special values does not
have the problem that a single special value has: how to distinguish
it from an actual value)

Then users can easily determine themselves:
is_set = value_min > value_max (or preferrably value_min==0 && value_max==-1)
is_range = value_min < value_max

So two questions:
1. What do you think of documenting case 3 instead of adding a new field to API?
2. What do you think of removing is_range?

Cheers,
Dee
diff mbox series

Patch

diff --git a/libavutil/opt.c b/libavutil/opt.c
index 4124efd9b6..ab127b30fa 100644
--- a/libavutil/opt.c
+++ b/libavutil/opt.c
@@ -1865,6 +1865,7 @@  int av_opt_query_ranges_default(AVOptionRanges **ranges_arg, void *obj, const ch
     ranges->nb_ranges = 1;
     ranges->nb_components = 1;
     range->is_range = 1;
+    range->is_set = 1;
     range->value_min = field->min;
     range->value_max = field->max;
 
diff --git a/libavutil/opt.h b/libavutil/opt.h
index c2329e5589..ac0b4567a6 100644
--- a/libavutil/opt.h
+++ b/libavutil/opt.h
@@ -325,6 +325,11 @@  typedef struct AVOptionRange {
      * If set to 1 the struct encodes a range, if set to 0 a single value.
      */
     int is_range;
+    /**
+     * Is set flag.
+     * If set to 1 the struct contains a value, if set to 0, the range is empty.
+     */
+    int is_set;
 } AVOptionRange;
 
 /**
diff --git a/libavutil/version.h b/libavutil/version.h
index e11eaa20d0..34b83112de 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@ 
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  57
-#define LIBAVUTIL_VERSION_MINOR   0
+#define LIBAVUTIL_VERSION_MINOR   1
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \