Adding allow_codec_switching parameter #726

sr1990 · 2020-03-09T00:07:45Z

This PR is related to #542 .
Adding allow_codec_switching=true, will allow switching between different codecs, if they have the same language, media type (audio, video etc) and container type.

$ packager \
  in=h264_baseline_360p_600.mp4,stream=audio,output=audio.mp4 \
  in=input_text.vtt,stream=text,output=output_text.vtt \
  in=h264_baseline_360p_600.mp4,stream=video,output=h264_360p.mp4 \
  in=h264_main_480p_1000.mp4,stream=video,output=h264_480p.mp4 \
  in=h264_main_720p_3000.mp4,stream=video,output=h264_720p.mp4 \
  in=h264_high_1080p_6000.mp4,stream=video,output=h264_1080p.mp4 \
  in=h265_high_1080p_6000.mp4,stream=video,output=h265_1080p.mp4 \ 
  --mpd_output h264.mpd --allow_codec_switching=true

kqyang

Thanks for working on this feature request. I left a few comments.

Also, can you add an end to end test in packager_test.py like you have done before in #721?

kqyang · 2020-03-10T01:01:30Z

packager/app/mpd_flags.cc

@@ -62,3 +62,8 @@ DEFINE_bool(
    "completely."
    "Ignored if $Time$ is used in segment template, since $Time$ requires "
    "accurate Segment Timeline.");
+DEFINE_bool(allow_codec_switching,


This block is not formatted correctly. Please follow either the blocks from line 49 - 52 or the blocks from line 53 - 64. Alternatively, you may need to use clang-format: https://github.com/google/shaka-packager/blob/master/packager/tools/git/check_formatting.py

kqyang · 2020-03-10T01:02:12Z

packager/app/mpd_flags.cc

+            false,
+	    "if enabled, allow switching between different codecs, if they "
+	    "have the same language, media type (audio, video etc) and "
+	    "container type");


Can you add a period, i.e. "." after type?

s/switching/adaptive switching/

kqyang · 2020-03-10T01:06:15Z

packager/mpd/base/adaptation_set.cc

@@ -165,12 +165,14 @@ class RepresentationStateChangeListenerImpl

 AdaptationSet::AdaptationSet(const std::string& language,
                             const MpdOptions& mpd_options,
-                             uint32_t* counter)
+                             uint32_t* counter,
+                             const MediaInfo& media_info)


Move "const MediaInfo& media_info" before "uint32_t* counter".

We follow Google C++ style guide, where input parameters are always placed before output parameters: https://google.github.io/styleguide/cppguide.html#Output_Parameters.

I thought about it again. This change is probably not needed if we don't plan to refactor out ProtectedContentMap.

Instead, what you need is to change line 85 of period.cc to include the bool flag.

@kqyang Just including the bool flag on line 85 will not create a new adaptation set along with switching supplemental property. I will take a look into refactoring out ProtectedContentMap. Please let me know if you have any pointers.

Just including the bool flag on line 85 will not create a new adaptation set along with switching supplemental property

There are a few additional changes associated with that. See below:

// AdaptationSets with the same key could differ in ContentProtection or codec (depends // on whether codec switching is allowed). const std::string key = GetAdaptationSetKey(media_info, mpd_options_.allow_codec_switching); std::list<AdaptationSet*>& adaptation_sets = adaptation_set_list_map_[key]; for (AdaptationSet* adaptation_set : adaptation_sets) { // TODO: Change Match to check if codecs is the same too. if (protected_adaptation_set_map_.Match(*adaptation_set, media_info)) return adaptation_set; } // None of the adaptation sets match with the new content protection. // Need a new one. const std::string language = GetLanguage(media_info); std::unique_ptr<AdaptationSet> new_adaptation_set = NewAdaptationSet(language, mpd_options_, representation_counter_); if (!SetNewAdaptationSetAttributes(language, media_info, adaptation_sets, new_adaptation_set.get())) { return nullptr; } if (content_protection_in_adaptation_set && media_info.has_protected_content()) { protected_adaptation_set_map_.Register(*new_adaptation_set, media_info); AddContentProtectionElements(media_info, new_adaptation_set.get()); } for (AdaptationSet* adaptation_set : adaptation_sets) { if (protected_adaptation_set_map_.Switchable(*adaptation_set, *new_adaptation_set)) { adaptation_set->AddAdaptationSetSwitching(new_adaptation_set.get()); new_adaptation_set->AddAdaptationSetSwitching(adaptation_set); } }

I will take a look into refactoring out ProtectedContentMap. Please let me know if you have any pointers.

That will be great. I think we can start with the change above and we can file a separate issue for the refactoring.

For comparing the codec in Match(), do you think adding codec as AdaptationSet member and setting it via SetNewAdaptationSetAttributes make sense or is there any other way to access codec info?

I just realized that you can store the codec in AdaptationSet::UpdateFromMediaInfo function.

kqyang · 2020-03-10T01:07:12Z

packager/mpd/base/adaptation_set.h

@@ -135,6 +135,9 @@ class AdaptationSet {
  /// @param id is the new ID to be set.
  void set_id(uint32_t id) { id_ = id; }

+  /// Get media info.
+  const MediaInfo& getMediaInfo() const { return media_info_;}


s/getMediaInfo/media_info/ to be consistent.

removed mediainfo from adaptation set

kqyang · 2020-03-10T01:10:47Z

packager/mpd/base/mpd_utils.cc

@@ -142,7 +142,7 @@ std::string GetBaseCodec(const MediaInfo& media_info) {
  return codec;
 }

-std::string GetAdaptationSetKey(const MediaInfo& media_info) {
+std::string GetAdaptationSetKey(const MediaInfo& media_info, bool no_codec) {


s/no_codec/ignore_codec/

kqyang · 2020-03-10T01:13:10Z

packager/mpd/base/period.h

@@ -130,7 +131,8 @@ class Period {
               const MediaInfo& media_info);
    // Check if the two adaptation sets are switchable.
    bool Switchable(const AdaptationSet& adaptation_set_a,
-                    const AdaptationSet& adaptation_set_b);
+                    const AdaptationSet& adaptation_set_b,
+                    const bool allow_codec_switching);


remove const here. We don't prefix primitive types with const in this code base.

kqyang · 2020-03-10T01:14:07Z

packager/mpd/base/period.h

@@ -88,7 +88,8 @@ class Period {
  virtual std::unique_ptr<AdaptationSet> NewAdaptationSet(
      const std::string& lang,
      const MpdOptions& options,
-      uint32_t* representation_counter);
+      uint32_t* representation_counter,
+      const MediaInfo& media_info);


Same as previous comment. Move media_info before representation_counter.

kqyang · 2020-03-10T01:48:35Z

packager/mpd/base/period.cc

@@ -112,12 +112,26 @@ AdaptationSet* Period::GetOrCreateAdaptationSet(

    for (AdaptationSet* adaptation_set : adaptation_sets) {
      if (protected_adaptation_set_map_.Switchable(*adaptation_set,
-                                                   *new_adaptation_set)) {
+                                                   *new_adaptation_set, false)) {


The code here shouldn't need to be updated if you don't plan to refactor ProtectedAdaptationSetMap out.

Instead, what you need is to change line 85 to include the bool flag.

kqyang · 2020-03-10T01:50:01Z

packager/mpd/base/mpd_utils.h

@@ -47,7 +47,8 @@ std::string GetCodecs(const MediaInfo& media_info);
 std::string GetBaseCodec(const MediaInfo& media_info);

 // Returns a key made from the characteristics that separate AdaptationSets.
-std::string GetAdaptationSetKey(const MediaInfo& media_info);
+std::string GetAdaptationSetKey(const MediaInfo& media_info,
+           bool no_codec = false);


We don't normally allow default parameters, which could hide problems. Please do not use default parameter here.

sr1990 · 2020-03-13T17:38:09Z

@kqyang CI jobs are failing. Please review and let me know.

kqyang

@sr1990 Good job! A few more comments. Appreciated your work on this change.

kqyang · 2020-03-13T17:42:14Z

packager/mpd/base/adaptation_set.h

@@ -183,7 +183,7 @@ class AdaptationSet {
  /// @param mpd_type is the type of this MPD.
  /// @param representation_counter is a Counter for assigning ID numbers to
  ///        Representation. It can not be NULL.
-  AdaptationSet(const std::string& language,
+   AdaptationSet(const std::string& language,


Remove the extra whitespace in the beginning.

kqyang · 2020-03-13T17:42:37Z

packager/mpd/base/adaptation_set.h

@@ -269,6 +269,9 @@ class AdaptationSet {
  // Determined by examining the MediaInfo passed to AddRepresentation().
  std::string content_type_;

+  // Codec of AdaptationSet


Add an ending period ".".

kqyang · 2020-03-13T17:43:51Z

packager/mpd/base/mpd_utils.h

@@ -47,7 +47,8 @@ std::string GetCodecs(const MediaInfo& media_info);
 std::string GetBaseCodec(const MediaInfo& media_info);

 // Returns a key made from the characteristics that separate AdaptationSets.
-std::string GetAdaptationSetKey(const MediaInfo& media_info);
+std::string GetAdaptationSetKey(const MediaInfo& media_info,
+           bool ignore_codec);


Align "bool" with "const". If they fit in the same line (<=80), move to the same line.

kqyang · 2020-03-13T17:45:05Z

packager/mpd/public/mpd_params.h

@@ -80,6 +80,9 @@ struct MpdParams {
  /// SegmentTimeline if it is enabled. It will be populated from segment
  /// duration specified in ChunkingParams if not specified.
  double target_segment_duration = 0;
+  ///If enabled, allow switching between different codecs, if they have the


Add a space before "If". Same below.

kqyang · 2020-03-13T19:53:26Z

packager/mpd/base/period.cc

@@ -82,19 +83,17 @@ AdaptationSet* Period::GetOrCreateAdaptationSet(
  // AdaptationSets with the same key should only differ in ContentProtection,
  // which also means that if |content_protection_in_adaptation_set| is false,
  // there should be at most one entry in |adaptation_sets|.


Can you update or remove this comment as it no longer applies?

kqyang · 2020-03-13T20:14:44Z

packager/mpd/base/adaptation_set.cc

@@ -412,10 +412,16 @@ void AdaptationSet::UpdateFromMediaInfo(const MediaInfo& media_info) {



Prefer just set

codec_ = GetCodecs(media_info);

kqyang · 2020-03-13T20:16:48Z

packager/mpd/base/period.cc

+              base::SPLIT_WANT_NONEMPTY)[0]) != 0)
+        return false;
+    }
+  }


Is SplitString needed?

with the change suggested above in line 412 of adaptation_set.cc, we can just do:

if (adaptation_set.codec() != GetCodecs(media_info))
return false;

GetCodecs returns the entire codec string (avc1.4d400d,avc1.4d401f, etc) if container type is not webm. Should I change the GetCodecs to return media_info.video_info().codec().substr(0, 4); for all container types or keep the SplitString in Match?

Ah, good catch, we shouldn't be using the entire codec string as two different representations in the same adaptation set could have two different codec strings.

Please replace GetCodecs with GetBaseCodec instead.

TrickPlayWithMatchingAdaptationSet unit test is failing as the codec in default_adaptation_set_ is not set because of no call to UpdateFromMediaInfo.
So, instead of setting codec from AdaptationSet::UpdateFromMediaInfo do you think it would make sense to set it at Period::SetNewAdaptationSetAttributes. If not, how would you change the unit test in this case? I tried calling the UpdateFromMediaInfo directly but that did not work.

Sure. You may add a AdaptationSet::set_codec(const std::string& codec) function and call it in Period::SetNewAdaptationSetAttributes.

(This tells me that we should really refactor the code later to clean up the code and remove ProtectedContentMap)

kqyang · 2020-03-13T20:19:08Z

packager/mpd/base/adaptation_set.h

@@ -269,6 +269,9 @@ class AdaptationSet {
  // Determined by examining the MediaInfo passed to AddRepresentation().
  std::string content_type_;

+  // Codec of AdaptationSet
+  std::string codec_;


Also define an accessor for codec_ after line 178:

/// @return codec.
std::string codec() const { return codec_; }

kqyang · 2020-03-13T20:27:35Z

packager/app/test/packager_test.py

+
+    self.assertPackageSuccess(streams, self._GetFlags(output_dash=True,allow_codec_switching=True))
+    self._CheckTestResults('audio-video-with-codec-switching')
+


Can you add another variant with encryption and trick play?

def testAllowCodecSwitchingWithEncryptionAndTrickplay(self): streams = [ self._GetStream('video', test_file='bear-640x360-hevc.mp4'), self._GetStream('video', test_file='bear-640x360.mp4'), self._GetStream('video', test_file='bear-1280x720.mp4'), self._GetStream('video', test_file='bear-1280x720.mp4', trick_play_factor=1), self._GetStream('audio', test_file='bear-640x360.mp4'), ] self.assertPackageSuccess(streams, self._GetFlags(output_dash=True, allow_codec_switching=True, encryption=True)) # Mpd cannot be validated right now since we don't generate determinstic # mpd with multiple inputs due to thread racing. # TODO(b/73349711): Generate determinstic mpd or at least validate mpd # schema. # See also https://github.com/google/shaka-packager/issues/177.

Ooops, I forgot one additional line here in my previous comment:

self._CheckTestResults( 'audio-video-with-codec-switching-encryption-trick-play', diff_files_policy=DiffFilesPolicy( allowed_diff_files=['output.mpd'], exact=False))

Similar for the test above:

self._CheckTestResults( 'audio-video-with-codec-switching', diff_files_policy=DiffFilesPolicy( allowed_diff_files=['output.mpd'], exact=False))

kqyang · 2020-03-13T20:36:35Z

packager/app/test/packager_test.py

+    ]
+
+    self.assertPackageSuccess(streams, self._GetFlags(output_dash=True,allow_codec_switching=True))
+    self._CheckTestResults('audio-video-with-codec-switching')


Unfortunately this line has to be removed as we don't generate determinstic mpd with multiple inputs right now.

Replace it with:

# Mpd cannot be validated right now since we don't generate determinstic # mpd with multiple inputs due to thread racing. # TODO(b/73349711): Generate determinstic mpd or at least validate mpd # schema. # See also https://github.com/google/shaka-packager/issues/177.

…t#728) It happens if `git` is not installed as a standard alone application, but as a shell command. Fixes shaka-project#724.

googlebot · 2020-03-17T02:53:19Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-03-17T02:53:19Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

sr1990 · 2020-03-17T02:55:58Z

@googlebot I consent.

kqyang

Looks very good to me. I have only a few more style comments. Thanks.

kqyang · 2020-03-17T05:44:10Z

packager/app/test/packager_test.py

+        self._GetStream('video', test_file='bear-640x360.mp4'),
+        self._GetStream('video', test_file='bear-1280x720.mp4'),
+        self._GetStream('video', test_file='bear-1280x720.mp4',
+                                           trick_play_factor=1),


I think trick_play_factor should align with 'video' above. You may use 'pyformat' to align it properly (there could be other formatting issues in this file, so it is fine to leave it as is for now and do it in a separate pull request or leave it to us to address).

kqyang · 2020-03-17T05:51:28Z

packager/mpd/base/adaptation_set.h

@@ -176,6 +176,13 @@ class AdaptationSet {
  /// @return true if it is a video AdaptationSet.
  bool IsVideo() const;

+  /// @return codec.
+  std::string codec() const { return codec_; }


Use const reference to avoid unnecessary copying:

const std::string& codec() const { return codec_; }

kqyang · 2020-03-17T05:51:57Z

packager/mpd/base/period.cc

@@ -7,6 +7,7 @@
 #include "packager/mpd/base/period.h"

 #include "packager/base/stl_util.h"
+#include "packager/base/strings/string_split.h"


Is this still needed?

no. removed it.

kqyang · 2020-03-17T05:53:32Z

packager/mpd/base/period.cc

@@ -176,7 +173,8 @@ bool Period::SetNewAdaptationSetAttributes(
    const std::string& language,
    const MediaInfo& media_info,
    const std::list<AdaptationSet*>& adaptation_sets,
-    AdaptationSet* new_adaptation_set) {
+    AdaptationSet* new_adaptation_set,
+    bool content_protection_in_adaptation_set) {


Swap the order of the last two parameters as Google C++ style guide requires the input argument to be placed before the output argument: https://google.github.io/styleguide/cppguide.html#Output_Parameters

kqyang · 2020-03-17T16:25:31Z

packager/mpd/public/mpd_params.h

@@ -80,6 +80,9 @@ struct MpdParams {
  /// SegmentTimeline if it is enabled. It will be populated from segment
  /// duration specified in ChunkingParams if not specified.
  double target_segment_duration = 0;
+  /// If enabled, allow switching between different codecs, if they have the
+  /// same language, media type (audio, video etc) and container type.
+  bool allow_codec_switching = false;


Normally we don't pull changes from master, which may cause confusions to the bots as seen in this PR. I'll see if I can merge it with this later.

sr1990 · 2020-08-24T22:12:36Z

@kqyang Do you think it makes sense to check and add audio channel count to adaptation set key in mpd_utils.cc:GetAdaptationSetKey when allow_codec_switching parameter is set to true ?

kqyang · 2020-08-27T04:19:36Z

Audio streams with different number of channels can be put in the same AdaptationSet, regardless of the allow_codec_switching flag, so I don't think it should be included in GetAdaptationSetKey. Let me know if you have a different opinion.

sr1990 · 2020-09-01T05:08:14Z

I think adaptive switch between audio with different channels won't be seamless.
On Exoplayer side, there is a flag allowAudioMixedChannelCountAdaptiveness in DefaultTrackSelector which is set to false by default and it filters out the audio track with appropriate channel count at: https://github.com/google/ExoPlayer/blob/e822196336b64208ba7caa557e4c6ee02b03335e/library/core/src/main/java/com/google/android/exoplayer2/trackselection/DefaultTrackSelector.java#L1647 and
as per issue google/ExoPlayer#6497:
"Also, by default, we won't allow adaptation between different channel counts (i.e. stereo and 5.1) because there may be an audible discontinuity when we switch formats."
This flag has been added in 2.10.5 as per https://github.com/google/ExoPlayer/blob/release-v2/RELEASENOTES.md.

So for players that do not handle this case, we could add a flag (--allow_audio_mixed_channel_count set to true by default) on the packager side to disable audio representations with different channel counts in the same adaptation set or we could let the players handle it. What do you think?

kqyang · 2020-09-01T18:30:21Z

@sr1990 So ExoPlayer already handles the case of a single AdaptationSet with audio streams of different number of channels, the feature for packager is not helpful for ExoPlayer.

It seems that it may create problems instead, as people would have to package two sets of contents if they want to enable adaptive switching between different audio streams in one platform but disable it in another platform.

sr1990 · 2020-09-02T06:29:12Z

It seems that it may create problems instead, as people would have to package two sets of contents if they want to enable adaptive switching between different audio streams in one platform but disable it in another platform
I agree. Thanks.

Adding allow_code_switching parameter

3ec315b

sr1990 changed the title ~~Adding allow_code_switching parameter~~ Adding allow_codec_switching parameter Mar 9, 2020

kqyang reviewed Mar 10, 2020

View reviewed changes

sr1990 added 2 commits March 13, 2020 00:39

Changes as per comments

7889bf6

Merge branch 'master' into allow_codec_switching

2fe2524

kqyang reviewed Mar 13, 2020

View reviewed changes

kqyang mentioned this pull request Mar 17, 2020

[DASH] Support TrickPlay using separate trick play specific streams #732

Closed

ab5y and others added 2 commits March 16, 2020 19:51

Fix git not found error on Windows when running gclient (shaka-projec…

6b13b67

…t#728) It happens if `git` is not installed as a standard alone application, but as a shell command. Fixes shaka-project#724.

Changes as per comments - 2

4344d71

kqyang reviewed Mar 17, 2020

View reviewed changes

Changes as per comments - 3

2524a17

kqyang reviewed Mar 17, 2020

View reviewed changes

Changes as per comments - 4

24b6d18

kqyang merged commit 55349aa into shaka-project:master Mar 18, 2020

kqyang mentioned this pull request Mar 18, 2020

[DASH] Allow AdaptationSet switching on FLAC and Opus (or make the codecs customize-able) #542

Closed

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

		@@ -412,10 +412,16 @@ void AdaptationSet::UpdateFromMediaInfo(const MediaInfo& media_info) {


		self.assertPackageSuccess(streams, self._GetFlags(output_dash=True,allow_codec_switching=True))
		self._CheckTestResults('audio-video-with-codec-switching')

Adding allow_codec_switching parameter #726

Adding allow_codec_switching parameter #726

Conversation

sr1990 commented Mar 9, 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

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

sr1990 commented Mar 13, 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

sr1990 Mar 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr1990 Mar 14, 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 Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Mar 17, 2020

googlebot commented Mar 17, 2020

sr1990 commented Mar 17, 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

sr1990 commented Aug 24, 2020

kqyang commented Aug 27, 2020

sr1990 commented Sep 1, 2020

kqyang commented Sep 1, 2020

sr1990 commented Sep 2, 2020

sr1990 Mar 14, 2020 •

edited

Loading

sr1990 Mar 14, 2020 •

edited

Loading

kqyang Mar 17, 2020 •

edited

Loading