[FFmpeg-devel] [PATCH 1/1] avformat/dashenc: Added #EXT-X-PROGRAM-DATE-TIME to HLS playlists

[Jeyapal, Karthick]

On 3/1/19 2:56 PM, joepadmiraal wrote:
> ---
>  libavformat/dashenc.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
Thanks for sending this revised patch. Now the patch looks fine overall.
There are two minor suggestions though.
>
> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
> index c5e882f4ae..4ee4a0cf72 100644
> --- a/libavformat/dashenc.c
> +++ b/libavformat/dashenc.c
> @@ -61,6 +61,7 @@ typedef struct Segment {
>      int64_t start_pos;
>      int range_length, index_length;
>      int64_t time;
> +    double prog_date_time;
>      int64_t duration;
>      int n;
>  } Segment;
> @@ -122,6 +123,7 @@ typedef struct DASHContext {
>      int64_t last_duration;
>      int64_t total_duration;
>      char availability_start_time[100];
> +    int64_t start_time_ms;
>      char dirname[1024];
>      const char *single_file_name;  /* file names as specified in options */
>      const char *init_seg_name;
> @@ -433,6 +435,8 @@ static void write_hls_media_playlist(OutputStream *os, AVFormatContext *s,
>      const char *proto = avio_find_protocol_name(c->dirname);
>      int use_rename = proto && !strcmp(proto, "file");
>      int i, start_index, start_number;
> +    time_t start_time_s = c->start_time_ms / 1000;
Is there any reason for not storing start_time_s directly in the DASHContext instead of start_time_ms?
Because two variables and two divisions seem redundant, when the same can be achieved with one. 
> +    double prog_date_time = 0;
>  
>      get_start_index_number(os, c, &start_index, &start_number);
>  
> @@ -467,11 +471,21 @@ static void write_hls_media_playlist(OutputStream *os, AVFormatContext *s,
>  
>      for (i = start_index; i < os->nb_segments; i++) {
>          Segment *seg = os->segments[i];
> +        double duration = (double) seg->duration / timescale;
> +
> +        if (prog_date_time == 0) {
> +            if (os->nb_segments == 1)
> +                prog_date_time = start_time_s;
> +            else
> +                prog_date_time = seg->prog_date_time;
> +        }
> +        seg->prog_date_time = prog_date_time;
> +
>          ret = ff_hls_write_file_entry(c->m3u8_out, 0, c->single_file,
> -                                (double) seg->duration / timescale, 0,
> +                                duration, 0,
This change looks unnecessary to the motive of this patch. Could this change be removed from this patch?
>                                  seg->range_length, seg->start_pos, NULL,
>                                  c->single_file ? os->initfile : seg->file,
> -                                NULL);
> +                                &prog_date_time);
>          if (ret < 0) {
>              av_log(os->ctx, AV_LOG_WARNING, "ff_hls_write_file_entry get error\n");
>          }
> @@ -1592,9 +1606,12 @@ static int dash_write_packet(AVFormatContext *s, AVPacket *pkt)
>          os->first_pts = pkt->pts;
>      os->last_pts = pkt->pts;
>  
> -    if (!c->availability_start_time[0])
> +    if (!c->availability_start_time[0]) {
> +        int64_t start_time_us = av_gettime();
> +        c->start_time_ms = start_time_us / 1000;
>          format_date_now(c->availability_start_time,
>                          sizeof(c->availability_start_time));
> +    }
>  
>      if (!os->availability_time_offset && pkt->duration) {
>          int64_t frame_duration = av_rescale_q(pkt->duration, st->time_base,