Skip to content
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 allow_codec_switching parameter #726

Merged
merged 7 commits into from
Mar 18, 2020
Merged

Adding allow_codec_switching parameter #726

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3ec315b
Adding allow_code_switching parameter
sr1990 Mar 9, 2020
7889bf6
Changes as per comments
sr1990 Mar 13, 2020
2fe2524
Merge branch 'master' into allow_codec_switching
sr1990 Mar 13, 2020
6b13b67
Fix git not found error on Windows when running gclient (#728)
ab5y Mar 13, 2020
4344d71
Changes as per comments - 2
sr1990 Mar 17, 2020
2524a17
Changes as per comments - 3
sr1990 Mar 17, 2020
24b6d18
Changes as per comments - 4
sr1990 Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packager/app/mpd_flags.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/switching/adaptive switching/

1 change: 1 addition & 0 deletions packager/app/mpd_flags.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ DECLARE_double(suggested_presentation_delay);
DECLARE_string(utc_timings);
DECLARE_bool(generate_dash_if_iop_compliant_mpd);
DECLARE_bool(allow_approximate_segment_timeline);
DECLARE_bool(allow_codec_switching);

#endif // APP_MPD_FLAGS_H_
1 change: 1 addition & 0 deletions packager/app/packager_main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,7 @@ base::Optional<PackagingParams> GetPackagingParams() {
FLAGS_generate_dash_if_iop_compliant_mpd;
mpd_params.allow_approximate_segment_timeline =
FLAGS_allow_approximate_segment_timeline;
mpd_params.allow_codec_switching = FLAGS_allow_codec_switching;

HlsParams& hls_params = packaging_params.hls_params;
if (!GetHlsPlaylistType(FLAGS_hls_playlist_type, &hls_params.playlist_type)) {
Expand Down
6 changes: 4 additions & 2 deletions packager/mpd/base/adaptation_set.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

: representation_counter_(counter),
language_(language),
mpd_options_(mpd_options),
segments_aligned_(kSegmentAlignmentUnknown),
force_set_segment_alignment_(false) {
force_set_segment_alignment_(false),
media_info_(media_info){
DCHECK(counter);
}

Expand Down
11 changes: 9 additions & 2 deletions packager/mpd/base/adaptation_set.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/getMediaInfo/media_info/ to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed mediainfo from adaptation set


/// Notifies the AdaptationSet instance that a new (sub)segment was added to
/// the Representation with @a representation_id.
/// This must be called every time a (sub)segment is added to a
Expand Down Expand Up @@ -183,9 +186,11 @@ 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,
/// @param media_info is media information of the adaptation set.
AdaptationSet(const std::string& language,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the extra whitespace in the beginning.

const MpdOptions& mpd_options,
uint32_t* representation_counter);
uint32_t* representation_counter,
const MediaInfo& media_info);

private:
AdaptationSet(const AdaptationSet&) = delete;
Expand Down Expand Up @@ -293,6 +298,8 @@ class AdaptationSet {
SegmentAligmentStatus segments_aligned_;
bool force_set_segment_alignment_;

const MediaInfo& media_info_;

// Keeps track of segment start times of Representations.
// For static MPD, this will not be cleared, all the segment start times are
// stored in this. This should not out-of-memory for a reasonable length
Expand Down
3 changes: 2 additions & 1 deletion packager/mpd/base/adaptation_set_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,13 @@ class AdaptationSetTest : public ::testing::Test {
public:
std::unique_ptr<AdaptationSet> CreateAdaptationSet(const std::string& lang) {
return std::unique_ptr<AdaptationSet>(
new AdaptationSet(lang, mpd_options_, &representation_counter_));
new AdaptationSet(lang, mpd_options_, &representation_counter_, media_info_));
}

protected:
MpdOptions mpd_options_;
uint32_t representation_counter_ = 0;
MediaInfo media_info_;
};

class OnDemandAdaptationSetTest : public AdaptationSetTest {
Expand Down
4 changes: 3 additions & 1 deletion packager/mpd/base/mock_mpd_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ namespace shaka {
namespace {
const char kEmptyLang[] = "";
const MpdOptions kDefaultMpdOptions;
const MediaInfo kDefaultMediaInfo;
} // namespace

// Doesn't matter what values get passed to the super class' constructor.
Expand All @@ -20,7 +21,8 @@ MockPeriod::MockPeriod(uint32_t period_id, double start_time_in_seconds)
&sequence_counter_) {}

MockAdaptationSet::MockAdaptationSet()
: AdaptationSet(kEmptyLang, kDefaultMpdOptions, &sequence_counter_) {}
: AdaptationSet(kEmptyLang, kDefaultMpdOptions, &sequence_counter_,
kDefaultMediaInfo) {}
MockAdaptationSet::~MockAdaptationSet() {}

MockRepresentation::MockRepresentation(uint32_t representation_id)
Expand Down
8 changes: 5 additions & 3 deletions packager/mpd/base/mpd_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/no_codec/ignore_codec/

std::string key;

if (media_info.has_video_info()) {
Expand All @@ -157,8 +157,10 @@ std::string GetAdaptationSetKey(const MediaInfo& media_info) {
}

key.append(MediaInfo_ContainerType_Name(media_info.container_type()));
key.append(":");
key.append(GetBaseCodec(media_info));
if (!no_codec) {
key.append(":");
key.append(GetBaseCodec(media_info));
}
key.append(":");
key.append(GetLanguage(media_info));

Expand Down
3 changes: 2 additions & 1 deletion packager/mpd/base/mpd_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


std::string SecondsToXmlDuration(double seconds);

Expand Down
4 changes: 3 additions & 1 deletion packager/mpd/base/mpd_utils_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@
namespace shaka {
namespace {
const char kNoLanguage[] = "";
const MediaInfo media_info;
} // namespace

class TestableAdaptationSet : public AdaptationSet {
public:
TestableAdaptationSet(const MpdOptions& mpd_options,
uint32_t* representation_counter)
: AdaptationSet(kNoLanguage, mpd_options, representation_counter) {}
: AdaptationSet(kNoLanguage, mpd_options, representation_counter,
media_info) {}
};

class MpdUtilsTest : public ::testing::Test {
Expand Down
37 changes: 32 additions & 5 deletions packager/mpd/base/period.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ AdaptationSet* Period::GetOrCreateAdaptationSet(
// Need a new one.
const std::string language = GetLanguage(media_info);
std::unique_ptr<AdaptationSet> new_adaptation_set =
NewAdaptationSet(language, mpd_options_, representation_counter_);
NewAdaptationSet(language, mpd_options_, representation_counter_, media_info);
if (!SetNewAdaptationSetAttributes(language, media_info, adaptation_sets,
new_adaptation_set.get())) {
return nullptr;
Expand All @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

adaptation_set->AddAdaptationSetSwitching(new_adaptation_set.get());
new_adaptation_set->AddAdaptationSetSwitching(adaptation_set);
}
}
}

if (mpd_options_.mpd_params.allow_codec_switching) {
for (auto a: adaptation_set_list_map_) {
std::list<AdaptationSet*>& adaptation_sets = a.second;
for (AdaptationSet* adaptation_set : adaptation_sets) {
if(protected_adaptation_set_map_.Switchable(*adaptation_set,
*new_adaptation_set, true)) {
new_adaptation_set->AddAdaptationSetSwitching(adaptation_set);
adaptation_set->AddAdaptationSetSwitching(new_adaptation_set.get());
}
}
}
}

AdaptationSet* adaptation_set_ptr = new_adaptation_set.get();
adaptation_sets.push_back(adaptation_set_ptr);
adaptation_sets_.emplace_back(std::move(new_adaptation_set));
Expand Down Expand Up @@ -167,9 +181,9 @@ const std::list<AdaptationSet*> Period::GetAdaptationSets() const {
std::unique_ptr<AdaptationSet> Period::NewAdaptationSet(
const std::string& language,
const MpdOptions& options,
uint32_t* representation_counter) {
uint32_t* representation_counter, const MediaInfo& media_info) {
return std::unique_ptr<AdaptationSet>(
new AdaptationSet(language, options, representation_counter));
new AdaptationSet(language, options, representation_counter, media_info));
}

bool Period::SetNewAdaptationSetAttributes(
Expand Down Expand Up @@ -275,7 +289,20 @@ bool Period::ProtectedAdaptationSetMap::Match(

bool Period::ProtectedAdaptationSetMap::Switchable(
const AdaptationSet& adaptation_set_a,
const AdaptationSet& adaptation_set_b) {
const AdaptationSet& adaptation_set_b,
const bool allow_codec_switching) {

if (allow_codec_switching) {
const MediaInfo& media_info_a = adaptation_set_a.getMediaInfo();
const MediaInfo& media_info_b = adaptation_set_b.getMediaInfo();

std::string key_a = GetAdaptationSetKey(media_info_a, true);
std::string key_b = GetAdaptationSetKey(media_info_b, true);

if (key_a == key_b)
return true;
}

const auto protected_content_it_a =
protected_content_map_.find(&adaptation_set_a);
const auto protected_content_it_b =
Expand Down
6 changes: 4 additions & 2 deletions packager/mpd/base/period.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comment. Move media_info before representation_counter.


// Helper function to set new AdaptationSet attributes.
bool SetNewAdaptationSetAttributes(
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private:
ProtectedAdaptationSetMap(const ProtectedAdaptationSetMap&) = delete;
Expand Down
Loading