[FFmpeg-devel] [PATCH] avformat/hls, dashdec: Don't use AV_OPT_FLAG_* in av_dict_set()

[FFmpeg-devel] [PATCH] avformat/hls, dashdec: Don't use AV_OPT_FLAG_* in av_dict_set()

[Andreas Rheinhardt]

av_dict_set() expects a different set of flags, namely the AV_DICT_*
flags. Using AV_OPT_FLAG_DECODING_PARAM (or any AV_OPT_FLAG_*) ic
av_dict_set() is therefore completely wrong and given that av_dict_set()
just doesn't care about whether the string it receives has anything
to do with a decoding parameter or not, it should just be removed
without replacement.
(The numerical value of AV_OPT_FLAG_DECODING_PARAM currently coincides
with AV_DICT_IGNORE_SUFFIX. Given that the dictionaries we are dealing
with here are always empty (i.e. NULL) before the calls to
av_dict_set(), this flag changes nothing. It would be different if
it were equal to one of the AV_DICT_DONT_STRDUP_* values.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/dashdec.c | 2 +-
 libavformat/hls.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
index 0f66251354..63bf7e96a5 100644
--- a/libavformat/dashdec.c
+++ b/libavformat/dashdec.c
@@ -1895,7 +1895,7 @@ static int reopen_demux_for_component(AVFormatContext *s, struct representation
     pls->ctx->io_open  = nested_io_open;
 
     if (c->cenc_decryption_key)
-        av_dict_set(&in_fmt_opts, "decryption_key", c->cenc_decryption_key, AV_OPT_FLAG_DECODING_PARAM);
+        av_dict_set(&in_fmt_opts, "decryption_key", c->cenc_decryption_key, 0);
 
     // provide additional information from mpd if available
     ret = avformat_open_input(&pls->ctx, "", in_fmt, &in_fmt_opts); //pls->init_section->url
diff --git a/libavformat/hls.c b/libavformat/hls.c
index 38eb346405..8204f55df3 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -2065,7 +2065,7 @@ static int hls_read_header(AVFormatContext *s)
             if (strstr(in_fmt->name, "mov")) {
                 char key[33];
                 ff_data_to_hex(key, pls->key, sizeof(pls->key), 0);
-                av_dict_set(&options, "decryption_key", key, AV_OPT_FLAG_DECODING_PARAM);
+                av_dict_set(&options, "decryption_key", key, 0);
             } else if (!c->crypto_ctx.aes_ctx) {
                 c->crypto_ctx.aes_ctx = av_aes_alloc();
                 if (!c->crypto_ctx.aes_ctx) {
-- 
2.32.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/hls, dashdec: Don't use AV_OPT_FLAG_* in av_dict_set()

[Jan Ekström]

On Sun, May 15, 2022 at 8:57 PM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> av_dict_set() expects a different set of flags, namely the AV_DICT_*
> flags. Using AV_OPT_FLAG_DECODING_PARAM (or any AV_OPT_FLAG_*) ic
> av_dict_set() is therefore completely wrong and given that av_dict_set()
> just doesn't care about whether the string it receives has anything
> to do with a decoding parameter or not, it should just be removed
> without replacement.
> (The numerical value of AV_OPT_FLAG_DECODING_PARAM currently coincides
> with AV_DICT_IGNORE_SUFFIX. Given that the dictionaries we are dealing
> with here are always empty (i.e. NULL) before the calls to
> av_dict_set(), this flag changes nothing. It would be different if
> it were equal to one of the AV_DICT_DONT_STRDUP_* values.)

LGTM as far as the patch contents go. Well noticed.

For commit message I think we utilize do not utilize capital letters
after the "prefix: ". This can be changed as you apply the patch :) .

Personally I would have worded it a la "don't use AV_OPT flags with
av_dict_set()", but this is just a preference and you are free to
ignore this bit.

Cheers,
Jan
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/hls, dashdec: Don't use AV_OPT_FLAG_* in av_dict_set()

[Andreas Rheinhardt]

Jan Ekström:
> On Sun, May 15, 2022 at 8:57 PM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
>>
>> av_dict_set() expects a different set of flags, namely the AV_DICT_*
>> flags. Using AV_OPT_FLAG_DECODING_PARAM (or any AV_OPT_FLAG_*) ic
>> av_dict_set() is therefore completely wrong and given that av_dict_set()
>> just doesn't care about whether the string it receives has anything
>> to do with a decoding parameter or not, it should just be removed
>> without replacement.
>> (The numerical value of AV_OPT_FLAG_DECODING_PARAM currently coincides
>> with AV_DICT_IGNORE_SUFFIX. Given that the dictionaries we are dealing
>> with here are always empty (i.e. NULL) before the calls to
>> av_dict_set(), this flag changes nothing. It would be different if
>> it were equal to one of the AV_DICT_DONT_STRDUP_* values.)
> 
> LGTM as far as the patch contents go. Well noticed.
> 
> For commit message I think we utilize do not utilize capital letters
> after the "prefix: ". This can be changed as you apply the patch :) .
> 

"we"? I always use capital letters here; so does Michael Niedermayer
(mostly). In the last two years, 4679 patches used a capital letter
after the ':', whereas 4409 used a non-capital letter. Granted, 3286 of
the 4679 are from me, but I don't think we have a policy here. Neither
should we.

> Personally I would have worded it a la "don't use AV_OPT flags with
> av_dict_set()", but this is just a preference and you are free to
> ignore this bit.

And I thought I wrote it in that way.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/hls, dashdec: Don't use AV_OPT_FLAG_* in av_dict_set()

[Jan Ekström]

On Sun, May 15, 2022 at 9:51 PM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Jan Ekström:
> > On Sun, May 15, 2022 at 8:57 PM Andreas Rheinhardt
> > <andreas.rheinhardt@outlook.com> wrote:
> >>
> >> av_dict_set() expects a different set of flags, namely the AV_DICT_*
> >> flags. Using AV_OPT_FLAG_DECODING_PARAM (or any AV_OPT_FLAG_*) ic
> >> av_dict_set() is therefore completely wrong and given that av_dict_set()
> >> just doesn't care about whether the string it receives has anything
> >> to do with a decoding parameter or not, it should just be removed
> >> without replacement.
> >> (The numerical value of AV_OPT_FLAG_DECODING_PARAM currently coincides
> >> with AV_DICT_IGNORE_SUFFIX. Given that the dictionaries we are dealing
> >> with here are always empty (i.e. NULL) before the calls to
> >> av_dict_set(), this flag changes nothing. It would be different if
> >> it were equal to one of the AV_DICT_DONT_STRDUP_* values.)
> >
> > LGTM as far as the patch contents go. Well noticed.
> >
> > For commit message I think we utilize do not utilize capital letters
> > after the "prefix: ". This can be changed as you apply the patch :) .
> >
>
> "we"? I always use capital letters here; so does Michael Niedermayer
> (mostly). In the last two years, 4679 patches used a capital letter
> after the ':', whereas 4409 used a non-capital letter. Granted, 3286 of
> the 4679 are from me, but I don't think we have a policy here. Neither
> should we.
>

Thanks for the stats. Somehow in my mind there was the mantra of "it
is not the start of a sentence, so do not capitalize it", and that
seemed to be the general consensus within the people I've mostly
communicated regarding this subject. The following paragraphs have
full sentences, which is why those would of course be capitalized.

Funny enough checking out
https://www.ffmpeg.org/developer.html#Patches_002fCommitting , the
example actually has a capital letter there in the short explanation
:) .

> > Personally I would have worded it a la "don't use AV_OPT flags with
> > av_dict_set()", but this is just a preference and you are free to
> > ignore this bit.
>
> And I thought I wrote it in that way.
>

Pretty much, in my mind just capitalizing AV_OPT and having flags as a
normal word just points towards the incorrect name space of the flags,
as opposed to AV_OPT_FLAG_* being a long fully capitalized identifier.
As I said, this is purely a preference thing.

Jan
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".