New command line argument: hls_media_sequence_number #702

Canta · 2020-01-23T13:42:09Z

See #691 for details on the use cases behind this.
Regarding the code changes:

A new command line argument is added.
media_sequence_number variable was changed to unsigned, as otherwise "big" media sequence numbers would end up negative, and there's no reason for this var not being unsigned.
There's also a new logic case for medialists that checks when this parameter is present and activates the discontinuity tag mechanics.

…sequence number offset.

googlebot · 2020-01-23T13:42:14Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-23T13:42:16Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-23T14:12:35Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-23T14:12:35Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Canta · 2020-01-23T14:12:36Z

@googlebot I fixed it.

kqyang

Thanks for the pull request. Generally looks good with a few misc comments and more questions.

kqyang · 2020-01-23T19:53:32Z

packager/hls/base/media_playlist.cc

@@ -390,6 +390,11 @@ bool MediaPlaylist::SetMediaInfo(const MediaInfo& media_info) {
  characteristics_ =
      std::vector<std::string>(media_info_.hls_characteristics().begin(),
                               media_info_.hls_characteristics().end());
+
+  if (hls_params_.media_sequence_number > media_sequence_number_) {


You can move it to MediaPlaylist constructor (line 346):

: hls_params_(hls_params), file_name_(file_name), name_(name), group_id_(group_id), media_sequence_number_(hls_params_.media_sequence_number) {}

Done and tested. Works fine by my side.

packager/hls/base/media_playlist.cc

kqyang · 2020-01-23T19:58:43Z

packager/hls/base/media_playlist.cc

+    hls_params_.media_sequence_number == media_sequence_number_) {
+    // When there's a forced media_sequence_number, start with discontinuity.
+    entries_.emplace_back(new DiscontinuityEntry());
+    inserted_discontinuity_tag_ = true;


This line can be removed. It is used to avoid inserting multiple discontinuity tag when there are multiple EXT-X-KEY tags.

I'm confused about this one. You mean the inserted_discontinuity_tag_ = true;?
I guess it was just a copypaste from the previously implemented discontinuity logic, but I kinda remember getting multiple discontinuity tags at some point. Are you sure about this?

No problem anyways, I'll do some tests to check this out.

Tested. It's ok to remove it.

kqyang · 2020-01-23T20:03:21Z

packager/app/hls_flags.cc

@@ -24,3 +24,6 @@ DEFINE_string(hls_playlist_type,
              "VOD, EVENT, or LIVE. This defines the EXT-X-PLAYLIST-TYPE in "
              "the HLS specification. For hls_playlist_type of LIVE, "
              "EXT-X-PLAYLIST-TYPE tag is omitted.");
+DEFINE_int32(hls_media_sequence_number,
+              0,
+              "Number. This defines the initial EXT-X-MEDIA-SEQUENCE value");


Can you add more background? e.g.

This defines the initial EXT-X-MEDIA-SEQUENCE value, which allows continuous media sequence across packager restarts. See #691 for more backgrounds.

Can you also update docs/source/options/hls_options.rst?

It is done here. Please take a look at it to check if you're ok with what I wrote (english is not my native language).

Canta · 2020-01-24T13:23:29Z

@kqyang Thank you very much for your time. I'll apply your changes as soon as I my agenda allows it. But just a quick question first. I'm a newbie in github's PRs. What's the procedure to alter the PR's code? I just modify my fork and create a new PR?

…ent)).

…ect#702 (comment)).

Canta · 2020-01-25T16:27:13Z

@kqyang I've applied modifications based on all your comments.

kqyang

Sorry for the delay in review. Mostly LGTM with a few more nit comments.

kqyang · 2020-01-24T19:46:48Z

docs/source/options/hls_options.rst

+    EXT-X-MEDIA-SEQUENCE value. This way, it's possible to continue the sequence
+    number from previous packager run.
+
+    See #691 for more information about the reasoning of this.


Can you replace #691 with `#691 <https://github.com/google/shaka-packager/issues/691>`_ as it is not automatically linked in our doc generator.

kqyang · 2020-01-30T01:44:36Z

packager/hls/public/hls_params.h

@@ -56,6 +56,8 @@ struct HlsParams {
  /// be populated from segment duration specified in ChunkingParams if not
  /// specified.
  double target_segment_duration = 0;
+  /// This allows to change the initial EXT-X-MEDIA-SEQUENCE field value.


Change to something like: (Also note 80 line length limit)

/// Custom EXT-X-MEDIA-SEQUENCE value to allow continuous media playback across packager restarts. See #691 for details.

kqyang · 2020-01-30T01:46:37Z

packager/hls/base/media_playlist.cc

+  } else if (hls_params_.media_sequence_number > 0 &&
+    hls_params_.media_sequence_number == media_sequence_number_) {
+    // When there's a forced media_sequence_number, start with discontinuity.
+    entries_.emplace_back(new DiscontinuityEntry());


It looks like we can move this block to the constructor, i.e. after line 347, which simplifies the logic a little bit:

group_id_(group_id), media_sequence_number_(hls_params_.media_sequence_number) {

// When there's a forced media_sequence_number, start with discontinuity.
if (media_sequence_number_ > 0)
entries_.emplace_back(new DiscontinuityEntry());
}

* shaka-project#702 (comment) * shaka-project#702 (comment) * shaka-project#702 (comment)

Canta · 2020-01-31T00:20:14Z

@kqyang Done! :)

kqyang

Thanks. There is still a small in the documentation.

kqyang · 2020-01-31T00:23:55Z

docs/source/options/hls_options.rst

+    number from previous packager run.
+
+    For more information about the reasoning of this, see issue #691:
+    `<https://github.com/google/shaka-packager/issues/691>`_


Sorry for not making it clearer. #691 should be moved after `. See examples here: https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#embedded-uris-and-aliases

Oh... it just felt wrong to have a large line (even when converted later), so I changed the text. My bad. I'll fix it right away.

Well... this is silly, but I guess it's better to explain myself. I'm gonna change it to this:

For more information about the reasoning of this, please see issue `#691 <https://github.com/google/shaka-packager/issues/691>`_

But that means later will get the issue number after a newline.
OTOH, if I put it this other way:

For more information about the reasoning of this, see `#691 <https://github.com/google/shaka-packager/issues/691>`_

It will end up nice in the docs, but ugly in the code.
Given that it would be the only long line I see in the docs, I'll go with option 1.

Yes, please go with option 1. New line in the code is not rendered in the docs :)

kqyang · 2020-01-31T00:25:09Z

docs/source/options/hls_options.rst

+    `<https://github.com/google/shaka-packager/issues/691>`_
+
+    The EXT-X-MEDIA-SEQUENCE documentation can be read here:
+    `<https://tools.ietf.org/html/rfc8216#section-4.3.3.2>`_


There is no need to quote it inside `` and <> as it does not have a text.

* shaka-project#702 (comment) * shaka-project#702 (comment)

kqyang · 2020-01-31T02:02:07Z

docs/source/options/hls_options.rst

+    number from previous packager run.
+
+    For more information about the reasoning of this, please see issue
+    `#691 <https://github.com/google/shaka-packager/issues/691>`_


Sorry, one more change. Can you include a period, i.e. "." in the end of both line 74 and line 77.

kqyang · 2020-01-31T02:40:34Z

@Canta Thanks for working on this! Have you tested and verified the changes work as expected?

I am going to merge the change once appveyor and travis complete the build.

Canta · 2020-01-31T13:03:38Z

@kqyang Well, I did some tests, but I'm ignorant about this project's testing pipeline, so IDK if my tests are valid. What I did was building the code and testing the details we've been discussing here. That worked fine by my side. My tests were:

Obviously, no warnings and/or compilation problems.
The new --hls_media_sequence_number parameter works as expected, even with the code changes (mostly "send stuff to the medialist constructor").
When the parameter is not passed, nothing weird happens, and shaka packager behaves as usual.

In fact, I have my fork working fine on a server since weeks from now, without any issue.
And last night I was struggling trying to understand quickly how to test the docs build locally, without success sadly. However, I could see the rst docs parsed in github's UI, and they did look fine.

Please tell me if you need some better validation than this.
Thanks!

kqyang · 2020-01-31T18:21:39Z

@Canta That is the answer I am expecting. Thanks a lot in bringing up the issue and addressing the issue. Appreciated!

kqyang · 2020-01-31T18:30:24Z

Btw, you may update https://github.com/google/shaka-packager/blob/master/CONTRIBUTORS and https://github.com/google/shaka-packager/blob/master/AUTHORS. It can be either a separate pull request or you can do it together with #701.

@kqyang

Also, added Daniel Cantarín to AUTHORS and CONTRIBUTORS files, as per @kqyang comment: shaka-project#702 (comment)

Daniel Cantarín added 2 commits December 31, 2019 16:14

Added ability to set by command line argument an arbitrary HLS media …

79a3d74

…sequence number offset.

fix: wrong condition placement.

bb0a329

kqyang reviewed Jan 23, 2020

View reviewed changes

Daniel Cantarín added 3 commits January 24, 2020 14:11

Changed help/doc strings as for @kqyang comments.

97950bb

Removed an unnecessary line, as stated [here](shaka-project#702 (comm…

3079acf

…ent)).

Sent variable to constructor, as per [code review comment](shaka-proj…

83dfc23

…ect#702 (comment)).

kqyang reviewed Jan 30, 2020

View reviewed changes

Several minor tweaks as for @kqyang comments:

9ad7ccc

* shaka-project#702 (comment) * shaka-project#702 (comment) * shaka-project#702 (comment)

kqyang reviewed Jan 31, 2020

View reviewed changes

Minor doc tweaks, as per review indications.

adeec48

* shaka-project#702 (comment) * shaka-project#702 (comment)

kqyang reviewed Jan 31, 2020

View reviewed changes

Added missing punctuation

bbae84c

kqyang merged commit 7aab7a8 into shaka-project:master Jan 31, 2020

Canta pushed a commit to Canta/shaka-packager that referenced this pull request Jan 31, 2020

Fix for shaka-project#701.

5ca890e

Also, added Daniel Cantarín to AUTHORS and CONTRIBUTORS files, as per @kqyang comment: shaka-project#702 (comment)

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New command line argument: hls_media_sequence_number #702

New command line argument: hls_media_sequence_number #702

Canta commented Jan 23, 2020

googlebot commented Jan 23, 2020

googlebot commented Jan 23, 2020

googlebot commented Jan 23, 2020

googlebot commented Jan 23, 2020

Canta commented Jan 23, 2020

kqyang left a comment

kqyang Jan 23, 2020

Canta Jan 25, 2020

kqyang Jan 23, 2020

Canta Jan 24, 2020

Canta Jan 25, 2020

kqyang Jan 23, 2020

kqyang Jan 23, 2020

Canta Jan 25, 2020

Canta commented Jan 24, 2020

Canta commented Jan 25, 2020

kqyang left a comment

kqyang Jan 24, 2020

kqyang Jan 30, 2020

kqyang Jan 30, 2020

Canta commented Jan 31, 2020

kqyang left a comment

kqyang Jan 31, 2020

Canta Jan 31, 2020

Canta Jan 31, 2020 •

edited

Loading

kqyang Jan 31, 2020

kqyang Jan 31, 2020

kqyang Jan 31, 2020

kqyang commented Jan 31, 2020

Canta commented Jan 31, 2020

kqyang commented Jan 31, 2020

kqyang commented Jan 31, 2020

New command line argument: hls_media_sequence_number #702

New command line argument: hls_media_sequence_number #702

Conversation

Canta commented Jan 23, 2020

googlebot commented Jan 23, 2020

googlebot commented Jan 23, 2020

googlebot commented Jan 23, 2020

googlebot commented Jan 23, 2020

Canta commented Jan 23, 2020

kqyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Canta commented Jan 24, 2020

Canta commented Jan 25, 2020

kqyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Canta commented Jan 31, 2020

kqyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Canta Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kqyang commented Jan 31, 2020

Canta commented Jan 31, 2020

kqyang commented Jan 31, 2020

kqyang commented Jan 31, 2020

Canta Jan 31, 2020 •

edited

Loading