[Dash] Support urn:mpeg:dash:adaptation-set-switching:2016 by matthuisman · Pull Request #694 · xbmc/inputstream.adaptive

matthuisman · 2021-05-19T04:43:05Z

fixes: #555

Note: This is not implementing adaptive bitrate switching....

Currently IA will just pick whichever video set is at the top of the file.
With this pr, it will merge whichever sets are allowed and therefore be able to access all the qualities either manually or automatically.

When adaptive switching is implemented, I'm pretty sure we will still need this PR so all representations are available for it to choose from

1st
Support urn:mpeg:dash:adaptation-set-switching:2016
Pretty straight-forward

store a list of ids (switching_ids_) pulled from urn:mpeg:dash:adaptation-set-switching:2016
this can be a single id, or multiple with comma delimiter
update SortTree() to also merge VIDEO sets
add new check to mergeable for VIDEOs with same group and both contain each others id in switching_ids_
add tests

2nd
Move the sorting to after merging sets
if we had merged, then less sets to sort = slightly faster

3rd
I also made the sort ignore type and language
it doesn't affect anything and makes the tests easier to view as they come in same order as the file.
Update the few tests that expected the order to change to the new ordered as in file
= less confusion

4th
💨 looks like a fart

TEST BUILDS HERE:
https://jenkins.kodi.tv/blue/organizations/jenkins/xbmc%2Finputstream.adaptive/detail/PR-694/7/artifacts

TEST STREAMS:
https://raw.githubusercontent.com/matthuisman/ia_tests/master/mpd_multi_vid_sets/mpd_multi_vid_sets_highest_first.strm
https://raw.githubusercontent.com/matthuisman/ia_tests/master/mpd_multi_vid_sets/mpd_multi_vid_sets_lowest_first.strm
https://raw.githubusercontent.com/matthuisman/ia_tests/master/mpd_multi_vid_sets/mpd_multi_vid_sets_different.strm

Only thing I wasn't sure about is:
https://github.com/xbmc/inputstream.adaptive/pull/694/files#diff-f5e439696055d30c282716f0d0fc54ee6612edb6e4f29ef91b4c3a00cd426679R618
that split() helper functions returns a new vector array.
is it ok to assign that directly to our std::vectorstd::string switching_ids_ ?

matthuisman · 2021-05-19T04:51:55Z

@kszaq
Want to give this a try and review please?

kszaq · 2021-05-19T07:15:01Z

Thank you, @matthuisman - the new code looks sane, I will give it a go this week.

If I may add my 2 cents, I would prefer if you kept the previous formatting in AdaptiveeTree.h, i.e. every condition on a new line. This helps readability and with && at the beginning of the line it's easier to blame when adding/removing conditions.

matthuisman · 2021-05-19T07:27:24Z

I'd argue that clang is based on best rules for readability for majority. Everyone has their own style they like, so I think it's best just to keep with clang? If we make an exception for this, then where does it stop? Oh, this also looks a bit nicer than clang thinks etc.

Its quite nice having something else make the decision for us :)

I do agree looks nicer the old way tho, so maybe @phunkyfish can advise if we can make exceptions? What would happen in the main repo in this case?

Unrelated note:
I also see potential for a bug. Think the existing code would segfault if adaption set had no representations.. But possibly only sets with representations make it to this point.

Looked into above - DashTree removes any adaptionsets without reprensentations here:
https://github.com/xbmc/inputstream.adaptive/blob/Matrix/src/parser/DASHTree.cpp#L1385
So looks like we are safe to access representations_[0] later on

phunkyfish · 2021-05-19T08:09:58Z

On clang from looking at the code it just appears unclean.

If there was a function to compare a and b I don’t think we’d be having this debate.

matthuisman · 2021-05-19T08:28:40Z

thats basically what this function is doing.
eg. we could change to something like this

    bool canMerge(const AdaptationSet *other)
    {
      if (type_ != other.type_)
        return false;

      if (type_ == VIDEO)
      {
        if (group_ != other->group_)
          return false;

        if (std::find(switching_ids_.begin(), switching_ids_.end(), other->id_) == switching_ids_.end())
          return false;

        if (std::find(other.begin(), other.end(), id_) == other.end())
          return false;

        return true;
      }
      else if(type_ == AUDIO)
      {
        if (timescale_ != other->timescale_)
          return false;

        if (duration_ != other->duration_)
          return false;

        13+ more if statements
      }

      return false;
    }

but then we just going to have a bunch of if statements?
What's more evil? Long single statement vs multiple statements all returning false?

I think even with the large if statement, the diff still will quite clearly show what is added / removed in the future?

matthuisman · 2021-05-19T08:58:02Z

ok, tried a bit more to get it look a bit cleaner and still clang
Hopefully a happy middle ground

kszaq · 2021-05-19T19:13:41Z

Checked with my (only) adaptation-set-switching provider: works as expected. Good job. 👍

kszaq · 2021-06-05T22:30:08Z

Hi @matthuisman. What is the plan regarding this PR?

matthuisman · 2021-06-05T23:33:25Z

Hi @matthuisman. What is the plan regarding this PR?

Pretty sure plan was to put it into Kodi N once it branches and only do bug fixes in Matrix. @glennguy can confirm

glennguy · 2021-06-06T11:34:05Z

A lot of these things are hard to put squarely in a 'bug fix' or 'feature' column.

Yes adding the support for the tag can be looked at as a 'feature' but also it fixes issues encountered when users play a playlists with these tags. Not show stopping of course and it can be worked around by a proxy.

Wrt to Matrix and N: branching for binary add-ons is imminent-ish and the idea is to hold on features, code cleanups/refactoring for the N branch with backports to follow into Matrix.

As for the code formatting - I agree with @phunkyfish, it looks bad. I can see that clang-format has BinPackArguments: true but I really think that it should be as the code that was removed, one line per argument. If you don't want clang-format to 'fix' it every time then you can wrap the section with // clang-format off and // clang-format on comments.

kszaq · 2021-06-06T21:05:59Z

What is the reason for feature freeze if N is far away? I understand this strategy for Kodi but as long as the addon API in Kodi stays the same, shouldn't new features be pushed also to Matrix branch? I can compile things so I don't care that much but I can imagine a PR with some brilliant new feature and users having to choose between using a stable Kodi without this new IA feature or having to switch to unstable Kodi nightly only to get an updated IA. I am fully aware that features (can) introduce bugs and that's why I feel distribution maintainers should keep at least one previous addon release available, then it's really simple to downgrade in case a new IA release breaks something.

phunkyfish · 2021-06-07T07:41:14Z

We are in the process of branching all the binary addons right now of which there are hundreds. So we avoid merging any new changes as we wait for this to complete.

Once that is done we then have place to test fixes more throughly before they go to Matrix.

Its up to maintainers whether or not to backport features however there is usually a time when so much refactoring has happened where the cost of this is too high.

glennguy · 2021-09-28T12:07:45Z

@matthuisman could you kindly rebase to resolve the confilct :)

phunkyfish · 2021-09-28T14:48:46Z

+        else if (strcmp(el, "SupplementalProperty") == 0)
+        {
+          const char *schemeIdUri(0), *value(0);
+
+          for (; *attr;)
+          {
+            if (strcmp((const char*)*attr, "schemeIdUri") == 0)
+              schemeIdUri = (const char*)*(attr + 1);
+            else if (strcmp((const char*)*attr, "value") == 0)
+              value = (const char*)*(attr + 1);
+            attr += 2;
+          }
+
+          if (schemeIdUri && value)
+          {
+            if (strcmp(schemeIdUri, "urn:mpeg:dash:adaptation-set-switching:2016") == 0)
+              dash->current_adaptationset_->switching_ids_ = split(value, ',');
+          }
+        }


Fully qualified calls to functions please, e.g. std::strcmp.

Also, C++ casts only.

i was just following the same code used above and below this
Can you please confirm you still want it changed?
Keep the same and then do a big refactor on all later?
Or fix as we go?

If it’s fixed as we go it’s less work for the big refactor. But @glennguy’s call as it will fall to him.

yeh, i'm just worried if he does a search and replace, having 1x different with a bunch of others may lead to a bad replace. Plus, I'm not too familiar with C++, so not sure what I'd change the casts too. haha

I'll sort these as part of the big refactor, for now lets get it in :)

matthuisman · 2021-09-28T22:03:10Z

rebased so no more conflics

This remove also adaptation set switching support "urn:mpeg:dash:adaptation-set-switching:2016" introduced by xbmc#694 the problem is that this implementation was eliminating adaptation sets with their associated data with related side effects, also as we currently have no way to determine which codecs are supported by the hardware as done corrupts the playback because can switch between differents codecs

matthuisman mentioned this pull request May 19, 2021

[DASH] Allow merging video adaption sets #689

Closed

glennguy approved these changes Jun 9, 2021

View reviewed changes

matthuisman closed this Jul 12, 2021

matthuisman deleted the adaption_set_switching branch July 12, 2021 21:03

matthuisman restored the adaption_set_switching branch July 12, 2021 21:03

matthuisman reopened this Jul 12, 2021

AlwinEsch changed the base branch from Matrix to Nexus September 14, 2021 14:31

glennguy requested a review from phunkyfish September 28, 2021 12:07

phunkyfish requested changes Sep 28, 2021

View reviewed changes

support urn:mpeg💨adaptation-set-switching:2016

415b540

glennguy merged commit 8b78183 into xbmc:Nexus Sep 28, 2021

matthuisman deleted the adaption_set_switching branch September 29, 2021 00:35

CastagnaIT mentioned this pull request Jul 28, 2023

[HLSTree] Reworked EXT-X-STREAM-INF parsing #1342

Closed

2 of 11 tasks

Conversation

matthuisman commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthuisman commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kszaq commented May 19, 2021

Uh oh!

matthuisman commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phunkyfish commented May 19, 2021

Uh oh!

matthuisman commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthuisman commented May 19, 2021

Uh oh!

kszaq commented May 19, 2021

Uh oh!

kszaq commented Jun 5, 2021

Uh oh!

matthuisman commented Jun 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennguy commented Jun 6, 2021

Uh oh!

kszaq commented Jun 6, 2021

Uh oh!

phunkyfish commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennguy commented Sep 28, 2021

Uh oh!

phunkyfish Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

matthuisman Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

phunkyfish Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

matthuisman Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glennguy Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

matthuisman commented Sep 28, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

matthuisman commented May 19, 2021 •

edited

Loading

matthuisman commented May 19, 2021 •

edited

Loading

matthuisman commented May 19, 2021 •

edited

Loading

matthuisman commented May 19, 2021 •

edited

Loading

matthuisman commented Jun 5, 2021 •

edited

Loading

phunkyfish commented Jun 7, 2021 •

edited

Loading

matthuisman Sep 28, 2021 •

edited

Loading