hls: Check for AVERROR_EOF in addition to the eof_reached flag

Message ID 1375008020-99796-1-git-send-email-martin@martin.st
State New
Headers show

Commit Message

Martin Storsjö July 28, 2013, 10:40 a.m.
From: Michael Niedermayer <michaelni@gmx.at>

---
 libavformat/hls.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Luca Barbato July 28, 2013, 10:46 a.m. | #1
On 28/07/13 12:40, Martin Storsjö wrote:
> From: Michael Niedermayer <michaelni@gmx.at>
> 
> ---
>  libavformat/hls.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 0d691ec..be0d551 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -610,7 +610,7 @@ start:
>                  AVStream *st;
>                  ret = av_read_frame(var->ctx, &var->pkt);
>                  if (ret < 0) {
> -                    if (!var->pb.eof_reached)
> +                    if (!var->pb.eof_reached && ret != AVERROR_EOF)
>                          return ret;
>                      reset_packet(&var->pkt);
>                      break;

Isn't one of the two redundant?

lu
Martin Storsjö July 28, 2013, 10:50 a.m. | #2
On Sun, 28 Jul 2013, Luca Barbato wrote:

> On 28/07/13 12:40, Martin Storsjö wrote:
>> From: Michael Niedermayer <michaelni@gmx.at>
>> 
>> ---
>>  libavformat/hls.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>> index 0d691ec..be0d551 100644
>> --- a/libavformat/hls.c
>> +++ b/libavformat/hls.c
>> @@ -610,7 +610,7 @@ start:
>>                  AVStream *st;
>>                  ret = av_read_frame(var->ctx, &var->pkt);
>>                  if (ret < 0) {
>> -                    if (!var->pb.eof_reached)
>> +                    if (!var->pb.eof_reached && ret != AVERROR_EOF)
>>                          return ret;
>>                      reset_packet(&var->pkt);
>>                      break;
>
> Isn't one of the two redundant?

In principle yes, but they're at slightly different levels. Since the 
return value is from a chained demuxer, the demuxer could in principle 
also return AVERROR_EOF even if the AVIOContext hasn't yet reached the 
end.

I'm not totally sure of the necessity for this patch so I can drop it if 
you prefer.

// Martin
Luca Barbato July 28, 2013, 10:59 a.m. | #3
On 28/07/13 12:50, Martin Storsjö wrote:
> In principle yes, but they're at slightly different levels. Since the
> return value is from a chained demuxer, the demuxer could in principle
> also return AVERROR_EOF even if the AVIOContext hasn't yet reached the end.

The main idea for that bit of code is that if you got your last packet
you want to forward it instead of losing it, right?

So checking for the AVERROR_EOF might be a bit more correct (even if the
two are synonyms for the case at hand).

I wonder how eagain should be considered now that you make me think.

lu
Derek Buitenhuis July 28, 2013, 11:03 a.m. | #4
On 7/28/2013 6:40 AM, Martin Storsjö wrote:
> From: Michael Niedermayer <michaelni@gmx.at>
> 
> ---
>  libavformat/hls.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

OK.

- Derek
Martin Storsjö July 28, 2013, 2:20 p.m. | #5
On Sun, 28 Jul 2013, Luca Barbato wrote:

> On 28/07/13 12:50, Martin Storsjö wrote:
>> In principle yes, but they're at slightly different levels. Since the
>> return value is from a chained demuxer, the demuxer could in principle
>> also return AVERROR_EOF even if the AVIOContext hasn't yet reached the end.
>
> The main idea for that bit of code is that if you got your last packet
> you want to forward it instead of losing it, right?

Almost - if one stream returns an error we'd just want to return this to 
the caller, but if one stream returns eof we've still got packets in the 
other streams to return.

> So checking for the AVERROR_EOF might be a bit more correct (even if the
> two are synonyms for the case at hand).
>
> I wonder how eagain should be considered now that you make me think.

I don't think eagain can happen here (we don't use nonblocking IO, and I 
don't think the mpegts demuxer ever returns that), but if it'd happen, we 
should just call av_read_frame again I think.


Anyway, I'm ok with deferring this patch until I've got a better 
explanation of why it's needed.

// Martin

Patch

diff --git a/libavformat/hls.c b/libavformat/hls.c
index 0d691ec..be0d551 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -610,7 +610,7 @@  start:
                 AVStream *st;
                 ret = av_read_frame(var->ctx, &var->pkt);
                 if (ret < 0) {
-                    if (!var->pb.eof_reached)
+                    if (!var->pb.eof_reached && ret != AVERROR_EOF)
                         return ret;
                     reset_packet(&var->pkt);
                     break;