[FFmpeg-devel] lavf/dashenc: Write media trailers when DASH trailer is written.

This commit ensures that all (potentially, long) filesystem activity is
performed when the user calls av_write_trailer on the DASH libavformat
context, not when freeing the context. Also, this defers media segment
deletion until after the media trailers are written.
---
 libavformat/dashenc.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

On 11/28/18 5:13 PM, Andrey Semashev wrote:
> This commit ensures that all (potentially, long) filesystem activity is

> performed when the user calls av_write_trailer on the DASH libavformat

> context, not when freeing the context. Also, this defers media segment

> deletion until after the media trailers are written.

> ---

>  libavformat/dashenc.c | 19 ++++++++++++++-----

>  1 file changed, 14 insertions(+), 5 deletions(-)

>

> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c

> index 6ce70e0076..e1c959dc89 100644

> --- a/libavformat/dashenc.c

> +++ b/libavformat/dashenc.c

> @@ -424,8 +424,6 @@ static void dash_free(AVFormatContext *s)

>          return;

>      for (i = 0; i < s->nb_streams; i++) {

>          OutputStream *os = &c->streams[i];

> -        if (os->ctx && os->ctx_inited)

> -            av_write_trailer(os->ctx);

>          if (os->ctx && os->ctx->pb)

>              ffio_free_dyn_buf(&os->ctx->pb);

>          ff_format_io_close(s, &os->out);

> @@ -1420,13 +1418,11 @@ static int dash_flush(AVFormatContext *s, int final, int stream)

>          os->pos += range_length;

>      }

>  

> -    if (c->window_size || (final && c->remove_at_exit)) {

> +    if (c->window_size) {

>          for (i = 0; i < s->nb_streams; i++) {

>              OutputStream *os = &c->streams[i];

>              int j;

>              int remove = os->nb_segments - c->window_size - c->extra_window_size;

> -            if (final && c->remove_at_exit)

> -                remove = os->nb_segments;

Is there any reason for deferring the delete after write_trailer.
Because if the file is getting deleted immediately, why should we bother about write_trailer? Is it causing any issues?
I am asking this, because the segment deletion code is getting duplicated due to this change. I am trying to avoid code duplication as much as possible.
>              if (remove > 0) {

>                  for (j = 0; j < remove; j++) {

>                      char filename[1024];

> @@ -1599,11 +1595,24 @@ static int dash_write_trailer(AVFormatContext *s)

>      }

>      dash_flush(s, 1, -1);

>  

> +    for (int i = 0; i < s->nb_streams; ++i) {

Can we merge this loop with the below loop for deleting init segments. The "if" condition for remove_at_exit, could be moved inside the merged loop. 
> +        OutputStream *os = &c->streams[i];

> +        if (os->ctx && os->ctx_inited) {

> +            av_write_trailer(os->ctx);

> +        }

> +    }

> +

>      if (c->remove_at_exit) {

>          char filename[1024];

>          int i;

>          for (i = 0; i < s->nb_streams; i++) {

>              OutputStream *os = &c->streams[i];

> +            for (int j = 0; j < os->nb_segments; ++j) {

> +                snprintf(filename, sizeof(filename), "%s%s", c->dirname, os->segments[j]->file);

> +                dashenc_delete_file(s, filename);

> +                av_free(os->segments[j]);

> +            }

> +            os->nb_segments = 0;

>              snprintf(filename, sizeof(filename), "%s%s", c->dirname, os->initfile);

>              dashenc_delete_file(s, filename);

>          }

On 11/29/18 9:27 AM, Jeyapal, Karthick wrote:
> 
> On 11/28/18 5:13 PM, Andrey Semashev wrote:
>> This commit ensures that all (potentially, long) filesystem activity is
>> performed when the user calls av_write_trailer on the DASH libavformat
>> context, not when freeing the context. Also, this defers media segment
>> deletion until after the media trailers are written.
>> ---
>>   libavformat/dashenc.c | 19 ++++++++++++++-----
>>   1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
>> index 6ce70e0076..e1c959dc89 100644
>> --- a/libavformat/dashenc.c
>> +++ b/libavformat/dashenc.c
>> @@ -424,8 +424,6 @@ static void dash_free(AVFormatContext *s)
>>           return;
>>       for (i = 0; i < s->nb_streams; i++) {
>>           OutputStream *os = &c->streams[i];
>> -        if (os->ctx && os->ctx_inited)
>> -            av_write_trailer(os->ctx);
>>           if (os->ctx && os->ctx->pb)
>>               ffio_free_dyn_buf(&os->ctx->pb);
>>           ff_format_io_close(s, &os->out);
>> @@ -1420,13 +1418,11 @@ static int dash_flush(AVFormatContext *s, int final, int stream)
>>           os->pos += range_length;
>>       }
>>   
>> -    if (c->window_size || (final && c->remove_at_exit)) {
>> +    if (c->window_size) {
>>           for (i = 0; i < s->nb_streams; i++) {
>>               OutputStream *os = &c->streams[i];
>>               int j;
>>               int remove = os->nb_segments - c->window_size - c->extra_window_size;
>> -            if (final && c->remove_at_exit)
>> -                remove = os->nb_segments;
> Is there any reason for deferring the delete after write_trailer.
> Because if the file is getting deleted immediately, why should we bother about write_trailer? Is it causing any issues?
> I am asking this, because the segment deletion code is getting duplicated due to this change. I am trying to avoid code duplication as much as possible.
>>               if (remove > 0) {
>>                   for (j = 0; j < remove; j++) {
>>                       char filename[1024];
>> @@ -1599,11 +1595,24 @@ static int dash_write_trailer(AVFormatContext *s)
>>       }
>>       dash_flush(s, 1, -1);
>>   
>> +    for (int i = 0; i < s->nb_streams; ++i) {
> Can we merge this loop with the below loop for deleting init segments. The "if" condition for remove_at_exit, could be moved inside the merged loop.

I usually don't like conditions on constants inside loops, but I can do 
that.

>> +        OutputStream *os = &c->streams[i];
>> +        if (os->ctx && os->ctx_inited) {
>> +            av_write_trailer(os->ctx);
>> +        }
>> +    }
>> +
>>       if (c->remove_at_exit) {
>>           char filename[1024];
>>           int i;
>>           for (i = 0; i < s->nb_streams; i++) {
>>               OutputStream *os = &c->streams[i];
>> +            for (int j = 0; j < os->nb_segments; ++j) {
>> +                snprintf(filename, sizeof(filename), "%s%s", c->dirname, os->segments[j]->file);
>> +                dashenc_delete_file(s, filename);
>> +                av_free(os->segments[j]);
>> +            }
>> +            os->nb_segments = 0;
>>               snprintf(filename, sizeof(filename), "%s%s", c->dirname, os->initfile);
>>               dashenc_delete_file(s, filename);
>>           }
>

On 11/29/18 9:27 AM, Jeyapal, Karthick wrote:
> 
> On 11/28/18 5:13 PM, Andrey Semashev wrote:
>> This commit ensures that all (potentially, long) filesystem activity is
>> performed when the user calls av_write_trailer on the DASH libavformat
>> context, not when freeing the context. Also, this defers media segment
>> deletion until after the media trailers are written.
>> ---
>>   libavformat/dashenc.c | 19 ++++++++++++++-----
>>   1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
>> index 6ce70e0076..e1c959dc89 100644
>> --- a/libavformat/dashenc.c
>> +++ b/libavformat/dashenc.c
>> @@ -424,8 +424,6 @@ static void dash_free(AVFormatContext *s)
>>           return;
>>       for (i = 0; i < s->nb_streams; i++) {
>>           OutputStream *os = &c->streams[i];
>> -        if (os->ctx && os->ctx_inited)
>> -            av_write_trailer(os->ctx);
>>           if (os->ctx && os->ctx->pb)
>>               ffio_free_dyn_buf(&os->ctx->pb);
>>           ff_format_io_close(s, &os->out);
>> @@ -1420,13 +1418,11 @@ static int dash_flush(AVFormatContext *s, int final, int stream)
>>           os->pos += range_length;
>>       }
>>   
>> -    if (c->window_size || (final && c->remove_at_exit)) {
>> +    if (c->window_size) {
>>           for (i = 0; i < s->nb_streams; i++) {
>>               OutputStream *os = &c->streams[i];
>>               int j;
>>               int remove = os->nb_segments - c->window_size - c->extra_window_size;
>> -            if (final && c->remove_at_exit)
>> -                remove = os->nb_segments;
> Is there any reason for deferring the delete after write_trailer.
> Because if the file is getting deleted immediately, why should we bother about write_trailer? Is it causing any issues?
> I am asking this, because the segment deletion code is getting duplicated due to this change. I am trying to avoid code duplication as much as possible.

This was partly in attempt to resolve the movenc errors caused by 
global_sidx. It did not completely remove the errors, but it seemed to 
have reduced them.

But mostly it is a logic sanity change. I believe it is incorrect to try 
writing a trailer to a non-existant file, and the downstream writer 
would be right to complain. If there is no file then you shouldn't be 
writing the trailer, but, AFAIK, that is not correct by libavformat 
usage protocol (i.e. you must write a trailer if you have written the 
header). Thus this change.

If you're worried about code duplication, I could move the segment 
deletion loop to a separate function.