New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding support for hls EXT_X_PROGRAM_DATE_TIME #840

Closed

r0ro wants to merge 1 commit into shaka-project:main from r0ro:add_x_program_date

r0ro commented Sep 12, 2020

Rework from #814

Adding support for generation of the EXT_X_PROGRAM_DATE_TIME tag for HLS playlists, with fixed UTC timezone

Add a new hls option: --hls_ext_x_program_date_time <time_offset_ms>

When turned on this option will insert EXT_X_PROGRAM_DATE_TIME in media playlists.
Date time is computed from local time adjusted by time_offset_ms.

ex playlist:

stream_0.m3u8

#EXTM3U
#EXT-X-VERSION:6
## Generated with https://github.com/google/shaka-packager version 81dc2360c2-release
#EXT-X-TARGETDURATION:8
#EXT-X-PLAYLIST-TYPE:VOD
#EXT-X-MAP:URI="360p.mp4",BYTERANGE="823@0"
#EXT-X-PROGRAM-DATE-TIME:2020-09-12T00:24:42.994Z
#EXTINF:8.000,
#EXT-X-BYTERANGE:531320@915
360p.mp4
#EXTINF:4.000,
#EXT-X-BYTERANGE:226186
360p.mp4
#EXTINF:8.000,
#EXT-X-BYTERANGE:543230
360p.mp4
#EXTINF:4.000,
#EXT-X-BYTERANGE:218737
360p.mp4
#EXTINF:4.040,
#EXT-X-BYTERANGE:207063
360p.mp4
#EXT-X-ENDLIST

stream_1.m3u8

#EXTM3U
#EXT-X-VERSION:6
## Generated with https://github.com/google/shaka-packager version 81dc2360c2-release
#EXT-X-TARGETDURATION:8
#EXT-X-PLAYLIST-TYPE:VOD
#EXT-X-MAP:URI="1080p.mp4",BYTERANGE="862@0"
#EXT-X-PROGRAM-DATE-TIME:2020-09-12T00:24:42.994Z
#EXTINF:8.000,
#EXT-X-BYTERANGE:2618974@954
1080p.mp4
#EXTINF:4.000,
#EXT-X-BYTERANGE:733878
1080p.mp4
#EXTINF:8.000,
#EXT-X-BYTERANGE:2170779
1080p.mp4
#EXTINF:4.000,
#EXT-X-BYTERANGE:710278
1080p.mp4
#EXTINF:4.040,
#EXT-X-BYTERANGE:514582
1080p.mp4
#EXT-X-ENDLIST

r0ro mentioned this pull request

Adding support for generation of the EXT_X_PROGRAM_DATE_TIME tag for … #814

Closed


          Adding support for generation of the EXT_X_PROGRAM_DATE_TIME tag for …

c6bc81f

…HLS playlists, with fixed UTC timezone

Add a new hls option: --hls_ext_x_program_date_time <time_offset_ms>

When turned on this option will insert EXT_X_PROGRAM_DATE_TIME
in media playlists. Date time is computed from local time adjusted
by time_offset_ms.

r0ro force-pushed the add_x_program_date branch from 931fabb to c6bc81f Compare

September 12, 2020 01:32

Collaborator

kqyang commented Sep 16, 2020

@r0ro Thanks for the PR. A bit busy right now. I'll try to review it later this week.

Author

r0ro commented Oct 3, 2020

@r0ro Thanks for the PR. A bit busy right now. I'll try to review it later this week.

Hi @kqyang did you get a chance to review this ?

kqyang reviewed

View reviewed changes

Collaborator

kqyang left a comment

@r0ro Sorry for the delay in the review. Super busy these days :(.

The general idea looks good to me. Thanks for working on it. I left some comments. Let me know if you need clarifications.

docs/source/options/hls_options.rst

@@ @@ -76,6 +76,13 @@ HLS options @@
                   The EXT-X-MEDIA-SEQUENCE documentation can be read here:
                   https://tools.ietf.org/html/rfc8216#section-4.3.3.2.
+              --hls_ext_x_program_date_time <time_offset_ms>

Collaborator

kqyang Oct 4, 2020

I don't have a definitive answer myself right now. I am thinking whether it is a good idea to reuse --suggested_presentation_delay from DASH for the offset in HLS instead having another offset here.

packager/app/packager_main.cc

@@ @@ -490,6 +490,11 @@ base::Optional<PackagingParams> GetPackagingParams() { @@
                 hls_params.default_text_language = FLAGS_default_text_language;
                 hls_params.media_sequence_number = FLAGS_hls_media_sequence_number;
+                if (FLAGS_hls_ext_x_program_date_time != INT32_MIN) {

Collaborator

kqyang Oct 4, 2020

What is the reason not to use 0 as default?

packager/hls/base/media_playlist.cc

@@ @@ -170,7 +172,8 @@ class SegmentInfoEntry : public HlsEntry { @@
                                  bool use_byte_range,
                                  uint64_t start_byte_offset,
                                  uint64_t segment_file_size,
-                                 uint64_t previous_segment_end_offset);
+                                 uint64_t previous_segment_end_offset,
+                                 const base::Time wall_time);

Collaborator

kqyang Oct 4, 2020

Pass by const reference to avoid copying, i.e. const base::Time& wall_time.

packager/hls/base/media_playlist.cc

@@ @@ -179,6 +182,10 @@ class SegmentInfoEntry : public HlsEntry { @@
                   duration_seconds_ = duration_seconds;
                 }
+                const base::Time wall_time() {

Collaborator

kqyang Oct 4, 2020

Return a const reference to avoid copying, i.e.

const base::Time& wall_time() const { return wall_time_; }

packager/hls/base/media_playlist.cc

                                              int64_t start_time,
                                              int64_t duration,
                                              uint64_t start_byte_offset,
-                                             uint64_t size) {
+                                             uint64_t size,
+                                             const base::Time reference_time) {

Collaborator

kqyang Oct 4, 2020

Since it only changes on discontinuity, it is probably a good idea to provide it in a separate function instead of in AddSegment.

packager/hls/base/media_playlist.cc

Comment on lines +495 to +515

+                  case HlsEntry::EntryType::kExtInf:
+                    if (need_ext_date_time) {
+                      SegmentInfoEntry* segment_info =
+                          reinterpret_cast<SegmentInfoEntry*>(entry.get());
+                      base::Time::Exploded time;
+                      segment_info->wall_time().UTCExplode(&time);
+                      base::StringAppendF(&content,
+                                          "#EXT-X-PROGRAM-DATE-TIME:"
+                                          "%4d-%02d-%02dT%02d:%02d:%02d.%03dZ\n",
+                                          time.year, time.month, time.day_of_month,
+                                          time.hour, time.minute, time.second,
+                                          time.millisecond);
+                      need_ext_date_time = false;
+                    }
+                    break;
+                  case HlsEntry::EntryType::kExtDiscontinuity:
+                    // we need to add a new program data time after a discontinuity
+                    need_ext_date_time = hls_params_.add_ext_x_program_date_time;
+                    break;
+                  default:
+                    break;

Collaborator

kqyang Oct 4, 2020

Indent by two extra characters.

packager/hls/base/media_playlist.cc

-                        << start_time << ".";
-                    entries_.emplace_back(new DiscontinuityEntry());
-                  }
+                const base::Time wall_time = reference_time + base::TimeDelta::FromSeconds(start_time / time_scale_);

Collaborator

kqyang Oct 4, 2020

I think you can defer the wall_time computation to MediaPlaylist::WriteToFile when writing the EXT-X-PROGRAM-DATE-TIME, then SegmentInfoEntry constructor does not need to be changed.

packager/hls/base/simple_hls_notifier.cc

Comment on lines +381 to +382

		base::Time adjusted_time = base::Time::Now()
		+ base::TimeDelta::FromMilliseconds(hls_params().packaging_time_offset_ms);

Collaborator

kqyang Oct 4, 2020

Let's assuming packaging_time_offset_ms to be 0 for now to simplify the illustration below.

I think we want to map Now() to the current segment which media timestamp start_time.

So the reference time should be: Now() - start_time. (If we set the reference time to Now(), the segment with start_time is mapped to Now() + start_time as start_time is added later)

There could be an offset, so the reference time or adjusted time is:

  base::Time::Now() + base::TimeDelta::FromMilliseconds(hls_params().packaging_time_offset_ms) - base::TimeDelta::FromSeconds(start_time / media_playlist->timescale());

packager/hls/base/simple_hls_notifier.cc

+                // Check if we need to set reference_time_
+                // This need to be adjusted on discontinuity
+                if (hls_params().add_ext_x_program_date_time) {
+                  if (reference_time_.is_null() || start_time < media_playlist->LastSegmentStartTime()) {

Collaborator

kqyang Oct 4, 2020

I like the idea of resetting the reference time when there are discontinuities!

There is a minor problem here though. Let's assume that there are two streams. The reference time will be reset twice for every discontinuity as the discontinuity exists in both streams.

Can you we always use one stream as reference (except for the initial case)?

The code can be structured as something like:

  if (hls_params().add_ext_x_program_date_time) {
    if (reference_time_.is_null()) {
      reference_time_ = CalculateReferenceTime(start_time);
      reference_stream_id_ = stream_id;
    } else if (stream_id == reference_stream_id_ && media_playlist->IsDiscontinuity(start_time)) {
      reference_time_ = CalculateReferenceTime(start_time);
    }
    media_playlist->SetReferenceTime(reference_time_);
  }

IsDiscontinuity(start_time) is a MediaPlaylist function which wraps start_time < media_playlist->LastSegmentStartTime(), as it is easier to understand and also easier to maintain in the future.

zuzzurro commented Jan 19, 2022

Are there any advances on this topic, by chance?

hariszukanovic commented Jan 26, 2024

Is this merge going to happen?

Decoydoll commented Feb 19, 2024

Hi @r0ro thank you for initiating this PR, are you still following up this PR? Currently i have a use case that is needing this feature, i hope this can be merged soon, thanks!

Author

r0ro commented Feb 19, 2024

Hi @r0ro thank you for initiating this PR, are you still following up this PR? Currently i have a use case that is needing this feature, i hope this can be merged soon, thanks!

I'm not sorry.
I believe this would need some rebase against current codebase and some modifications as suggested by @kqyang

Collaborator

cosmin commented Apr 30, 2024

Going to close this for now since nobody is actively working on it, if someone else wants to pick it up please open a new PR on top of the current main branch.

cosmin closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment