[FFmpeg-devel] [PATCH 2/3] avformat/hls: Show error info when read key file failed

[FFmpeg-devel] [PATCH 2/3] avformat/hls: Show error info when read key file failed

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

---
 libavformat/hls.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/libavformat/hls.c b/libavformat/hls.c
index 6623c80309..45c1b283c9 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -1353,16 +1353,18 @@ static int open_input(HLSContext *c, struct playlist *pls, struct segment *seg,
     if (seg->key_type == KEY_AES_128 || seg->key_type == KEY_SAMPLE_AES) {
         if (strcmp(seg->key, pls->key_url)) {
             AVIOContext *pb = NULL;
-            if (open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL) == 0) {
+
+            ret = open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL);
+            if (ret >= 0) {
                 ret = avio_read(pb, pls->key, sizeof(pls->key));
                 if (ret != sizeof(pls->key)) {
-                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s\n",
-                           seg->key);
+                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s, %s\n",
+                           seg->key, av_err2str(ret));
                 }
                 ff_format_io_close(pls->parent, &pb);
             } else {
-                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s\n",
-                       seg->key);
+                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s, %s\n",
+                       seg->key, av_err2str(ret));
             }
             av_strlcpy(pls->key_url, seg->key, sizeof(pls->key_url));
         }
-- 
2.46.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 2/3] avformat/hls: Show error info when read key file failed

[Steven Liu]

> On Apr 23, 2025, at 20:59, Zhao Zhili <quinkblack-at-foxmail.com@ffmpeg.org> wrote:
Hi Zhili,
> 
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> ---
> libavformat/hls.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 6623c80309..45c1b283c9 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -1353,16 +1353,18 @@ static int open_input(HLSContext *c, struct playlist *pls, struct segment *seg,
>     if (seg->key_type == KEY_AES_128 || seg->key_type == KEY_SAMPLE_AES) {
>         if (strcmp(seg->key, pls->key_url)) {
>             AVIOContext *pb = NULL;
> -            if (open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL) == 0) {
> +
> +            ret = open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL);
> +            if (ret >= 0) {
>                 ret = avio_read(pb, pls->key, sizeof(pls->key));
>                 if (ret != sizeof(pls->key)) {
> -                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s\n",
> -                           seg->key);
> +                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s, %s\n",
> +                           seg->key, av_err2str(ret));
>                 }
>                 ff_format_io_close(pls->parent, &pb);
>             } else {
> -                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s\n",
> -                       seg->key);
> +                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s, %s\n",
> +                       seg->key, av_err2str(ret));
>             }
Why not jump to cleanup immediately after an output error?

Thanks
Steven
_______________________________________________
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 2/3] avformat/hls: Show error info when read key file failed

[Zhao Zhili]

> On Apr 24, 2025, at 10:24, Steven Liu <lq@chinaffmpeg.org> wrote:
> 
> 
>> On Apr 23, 2025, at 20:59, Zhao Zhili <quinkblack-at-foxmail.com@ffmpeg.org> wrote:
> Hi Zhili,
>> 
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> ---
>> libavformat/hls.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>> 
>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>> index 6623c80309..45c1b283c9 100644
>> --- a/libavformat/hls.c
>> +++ b/libavformat/hls.c
>> @@ -1353,16 +1353,18 @@ static int open_input(HLSContext *c, struct playlist *pls, struct segment *seg,
>>    if (seg->key_type == KEY_AES_128 || seg->key_type == KEY_SAMPLE_AES) {
>>        if (strcmp(seg->key, pls->key_url)) {
>>            AVIOContext *pb = NULL;
>> -            if (open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL) == 0) {
>> +
>> +            ret = open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL);
>> +            if (ret >= 0) {
>>                ret = avio_read(pb, pls->key, sizeof(pls->key));
>>                if (ret != sizeof(pls->key)) {
>> -                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s\n",
>> -                           seg->key);
>> +                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s, %s\n",
>> +                           seg->key, av_err2str(ret));
>>                }
>>                ff_format_io_close(pls->parent, &pb);
>>            } else {
>> -                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s\n",
>> -                       seg->key);
>> +                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s, %s\n",
>> +                       seg->key, av_err2str(ret));
>>            }
> Why not jump to cleanup immediately after an output error?

Because I don't know why not return error when the code is added in 84465f21 since 2011, and not
changed since then. I'm worried return error might trigger unknown bugs.

Cc Martin. Is there some reason not to error out in this case? Does that reason still hold now?

> 
> Thanks
> Steven
> _______________________________________________
> 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".

_______________________________________________
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 2/3] avformat/hls: Show error info when read key file failed

[Marvin Scholz]

On 23 Apr 2025, at 14:59, Zhao Zhili wrote:

> From: Zhao Zhili <zhilizhao@tencent.com>
>
> ---
>  libavformat/hls.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 6623c80309..45c1b283c9 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -1353,16 +1353,18 @@ static int open_input(HLSContext *c, struct playlist *pls, struct segment *seg,
>      if (seg->key_type == KEY_AES_128 || seg->key_type == KEY_SAMPLE_AES) {
>          if (strcmp(seg->key, pls->key_url)) {
>              AVIOContext *pb = NULL;
> -            if (open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL) == 0) {
> +
> +            ret = open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL);
> +            if (ret >= 0) {
>                  ret = avio_read(pb, pls->key, sizeof(pls->key));
>                  if (ret != sizeof(pls->key)) {
> -                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s\n",
> -                           seg->key);
> +                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s, %s\n",
> +                           seg->key, av_err2str(ret));

The check here is just checking if the size matches the expected size, lets say it just
reads 1 byte instead if 10, av_err2str(ret) called with 1 in this example wont give you
anything meaningful and even more unhelpful than the previous message.

You should check that ret is actually negative, else you should probably report that the file
ended unexpectedly, printing the expected size and amount that was actually read.

(Also unrelated to this patch but this ret is eventually returned by the function with a possibly
positive value in this case which seems wrong...)

>                  }
>                  ff_format_io_close(pls->parent, &pb);
>              } else {
> -                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s\n",
> -                       seg->key);
> +                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s, %s\n",
> +                       seg->key, av_err2str(ret));
>              }
>              av_strlcpy(pls->key_url, seg->key, sizeof(pls->key_url));
>          }
> -- 
> 2.46.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".
_______________________________________________
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".

[FFmpeg-devel] [PATCH v2 2/3] avformat/hls: Show error info when read key file failed

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

---
 libavformat/hls.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/libavformat/hls.c b/libavformat/hls.c
index 6623c80309..6139b8525e 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -1353,16 +1353,22 @@ static int open_input(HLSContext *c, struct playlist *pls, struct segment *seg,
     if (seg->key_type == KEY_AES_128 || seg->key_type == KEY_SAMPLE_AES) {
         if (strcmp(seg->key, pls->key_url)) {
             AVIOContext *pb = NULL;
-            if (open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL) == 0) {
+
+            ret = open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL);
+            if (ret >= 0) {
                 ret = avio_read(pb, pls->key, sizeof(pls->key));
                 if (ret != sizeof(pls->key)) {
-                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s\n",
-                           seg->key);
+                    if (ret < 0)
+                        av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s, %s\n",
+                               seg->key, av_err2str(ret));
+                    else
+                        av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s, read bytes %d != %zu\n",
+                               seg->key, ret, sizeof(pls->key));
                 }
                 ff_format_io_close(pls->parent, &pb);
             } else {
-                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s\n",
-                       seg->key);
+                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s, %s\n",
+                       seg->key, av_err2str(ret));
             }
             av_strlcpy(pls->key_url, seg->key, sizeof(pls->key_url));
         }
-- 
2.46.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 2/3] avformat/hls: Show error info when read key file failed

[Zhao Zhili]

> On Apr 24, 2025, at 11:08, Marvin Scholz <epirat07@gmail.com> wrote:
> 
> 
> On 23 Apr 2025, at 14:59, Zhao Zhili wrote:
> 
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> ---
>> libavformat/hls.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>> 
>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>> index 6623c80309..45c1b283c9 100644
>> --- a/libavformat/hls.c
>> +++ b/libavformat/hls.c
>> @@ -1353,16 +1353,18 @@ static int open_input(HLSContext *c, struct playlist *pls, struct segment *seg,
>>     if (seg->key_type == KEY_AES_128 || seg->key_type == KEY_SAMPLE_AES) {
>>         if (strcmp(seg->key, pls->key_url)) {
>>             AVIOContext *pb = NULL;
>> -            if (open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL) == 0) {
>> +
>> +            ret = open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL);
>> +            if (ret >= 0) {
>>                 ret = avio_read(pb, pls->key, sizeof(pls->key));
>>                 if (ret != sizeof(pls->key)) {
>> -                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s\n",
>> -                           seg->key);
>> +                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s, %s\n",
>> +                           seg->key, av_err2str(ret));
> 
> The check here is just checking if the size matches the expected size, lets say it just
> reads 1 byte instead if 10, av_err2str(ret) called with 1 in this example wont give you
> anything meaningful and even more unhelpful than the previous message.
> 
> You should check that ret is actually negative, else you should probably report that the file
> ended unexpectedly, printing the expected size and amount that was actually read.

Alright, fixed in v2.

https://ffmpeg.org/pipermail/ffmpeg-devel/2025-April/342706.html

> 
> (Also unrelated to this patch but this ret is eventually returned by the function with a possibly
> positive value in this case which seems wrong...)

This doesn’t happen now since the code doesn't error out when read key failed.

I guess for key type SAMPLE_AES, audio track can be in clear, while the video track can be
partially encrypted.

There is a check at line 1408 which should be ret >= 0, but > 0 doesn’t happen in real.

1408     if (ret == 0 && !is_http && seg->url_offset) {


> 
>>                 }
>>                 ff_format_io_close(pls->parent, &pb);
>>             } else {
>> -                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s\n",
>> -                       seg->key);
>> +                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s, %s\n",
>> +                       seg->key, av_err2str(ret));
>>             }
>>             av_strlcpy(pls->key_url, seg->key, sizeof(pls->key_url));
>>         }
>> -- 
>> 2.46.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".
> _______________________________________________
> 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".

_______________________________________________
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 v2 2/3] avformat/hls: Show error info when read key file failed

[Marvin Scholz]

On 24 Apr 2025, at 5:54, Zhao Zhili wrote:

> From: Zhao Zhili <zhilizhao@tencent.com>
>
> ---
>  libavformat/hls.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 6623c80309..6139b8525e 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -1353,16 +1353,22 @@ static int open_input(HLSContext *c, struct playlist *pls, struct segment *seg,
>      if (seg->key_type == KEY_AES_128 || seg->key_type == KEY_SAMPLE_AES) {
>          if (strcmp(seg->key, pls->key_url)) {
>              AVIOContext *pb = NULL;
> -            if (open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL) == 0) {
> +
> +            ret = open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL);
> +            if (ret >= 0) {
>                  ret = avio_read(pb, pls->key, sizeof(pls->key));
>                  if (ret != sizeof(pls->key)) {
> -                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s\n",
> -                           seg->key);
> +                    if (ret < 0)
> +                        av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s, %s\n",
> +                               seg->key, av_err2str(ret));
> +                    else
> +                        av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s, read bytes %d != %zu\n",
> +                               seg->key, ret, sizeof(pls->key));
>                  }
>                  ff_format_io_close(pls->parent, &pb);
>              } else {
> -                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s\n",
> -                       seg->key);
> +                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s, %s\n",
> +                       seg->key, av_err2str(ret));
>              }
>              av_strlcpy(pls->key_url, seg->key, sizeof(pls->key_url));
>          }
> -- 
> 2.46.0

LGTM

>
> _______________________________________________
> 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".
_______________________________________________
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 2/3] avformat/hls: Show error info when read key file failed

[Martin Storsjö]

On Thu, 24 Apr 2025, Zhao Zhili wrote:

>> On Apr 24, 2025, at 10:24, Steven Liu <lq@chinaffmpeg.org> wrote:
>>
>>
>>> On Apr 23, 2025, at 20:59, Zhao Zhili <quinkblack-at-foxmail.com@ffmpeg.org> wrote:
>> Hi Zhili,
>>>
>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>>
>>> ---
>>> libavformat/hls.c | 12 +++++++-----
>>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>>> index 6623c80309..45c1b283c9 100644
>>> --- a/libavformat/hls.c
>>> +++ b/libavformat/hls.c
>>> @@ -1353,16 +1353,18 @@ static int open_input(HLSContext *c, struct playlist *pls, struct segment *seg,
>>>    if (seg->key_type == KEY_AES_128 || seg->key_type == KEY_SAMPLE_AES) {
>>>        if (strcmp(seg->key, pls->key_url)) {
>>>            AVIOContext *pb = NULL;
>>> -            if (open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL) == 0) {
>>> +
>>> +            ret = open_url(pls->parent, &pb, seg->key, &c->avio_opts, NULL, NULL);
>>> +            if (ret >= 0) {
>>>                ret = avio_read(pb, pls->key, sizeof(pls->key));
>>>                if (ret != sizeof(pls->key)) {
>>> -                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s\n",
>>> -                           seg->key);
>>> +                    av_log(pls->parent, AV_LOG_ERROR, "Unable to read key file %s, %s\n",
>>> +                           seg->key, av_err2str(ret));
>>>                }
>>>                ff_format_io_close(pls->parent, &pb);
>>>            } else {
>>> -                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s\n",
>>> -                       seg->key);
>>> +                av_log(pls->parent, AV_LOG_ERROR, "Unable to open key file %s, %s\n",
>>> +                       seg->key, av_err2str(ret));
>>>            }
>> Why not jump to cleanup immediately after an output error?
>
> Because I don't know why not return error when the code is added in 84465f21 since 2011, and not
> changed since then. I'm worried return error might trigger unknown bugs.
>
> Cc Martin. Is there some reason not to error out in this case? Does that reason still hold now?

I really don't remember if there was any specific reason for not wanting 
to error out directly at that point - I wouldn't think so.

So feel free to add that if it makes sense; it's most likely an oversight.

// Martin

_______________________________________________
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".