| Message ID | DB6PR0101MB22147298155FC32D2B9696E08FCC9@DB6PR0101MB2214.eurprd01.prod.exchangelabs.com |
|---|---|
| State | Accepted |
| Commit | e3580f60775c897c3b13b178c57ab191ecc4a031 |
| Headers | show |
| Series | [FFmpeg-devel] avformat/hls, dashdec: Don't use AV_OPT_FLAG_* in av_dict_set() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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
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
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
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) {
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(-)