Channel count as an input feature #84

mariocynicys · 2021-07-21T17:09:22Z

This PR removes the channels field from the pipeline configuration and replaces it with channel_layouts, a list of channel layouts to encode.
It also adds channel_layout field for Input class.

mariocynicys · 2021-07-21T17:13:30Z

We can also follow the VideoResolutionName approach and make an AudioChannelLayoutName and have a channel_layout field instead where the user can type in stereo, mono, etc..

joeyparrish · 2021-07-21T17:20:10Z

streamer/input_configuration.py

@@ -146,6 +146,9 @@ class Input(configuration.Base):
  Otherwise, it will default to 'und' (undetermined).
  """

+  channels = configuration.Field(int).cast()
+  """The number of audio channels to encode."""


It's more like "The number of channels in an audio input", right? It's less about encoding, and more about the structure of the input. Do I understand that right?

No, i feel a bit confused because of this too.
This is actually an output feature, the number of channels to encode in the final output file. sticking it with the each input(instead of the pipeline config) will allow us to give different channel counts for each input in a multi-period list.

Another less confusing approach could be, to deal with it like resolutions, we may specify the channel counts we want as an output to be encoded with, and it's a list of channel counts rather than a single channel count and each input will be encoded with different channel counts up till its own channel count just like video resolutions.

That would make sense to me. I like having it in the input, though, with auto-detection, just like resolution and frame rate for video inputs. If auto-detect fails, or if the input is coming from a pipe, rather than a file, we would need to be able to specify it in the input config.

So let's keep what you have (but fix the comment here on the field meaning), and then let's add a list of output channel counts in the pipeline config, just like we do for resolutions.

…r is raised" This reverts commit 9bd8c7e.

…ator

joeyparrish · 2021-07-30T20:20:26Z

streamer/configuration.py

@@ -561,5 +561,5 @@ def validate(cls, key):

    if key not in cls.map_class.keys():
      raise ValueError(
-          '{} is not a valid {}'.format(key, cls.map_class.__name__))
+          '{} is not a valid {}, {}'.format(key, cls.map_class.__name__, cls.name()))


From what I can tell re-reading the source, this now repeats cls.map_class.__name__, which is also printed by name() above:

"foo is not a valid VideoResolution, VideoResolution name (one of 720p, 1080p, ...)"

Should we drop cls.map_class.__name__ in favor of cls.name()?

You read that write.
Yes i think we should drop it in favor of cls.name().

joeyparrish · 2021-07-30T20:39:09Z

streamer/pipeline_configuration.py

-  """The number of audio channels to encode."""
+  channel_layouts = configuration.Field(
+      List[bitrate_configuration.AudioChannelLayoutName],
+      required=True).cast()


I don't agree with making this a required field. I think we should have a default value instead. Perhaps ['surround', 'stereo']. Not sure. What are your thoughts?

I fear that someone might provide a generic bitrate config that doesn't have stereo in it.

My thoughts is that we can default to a list that contains only the lowest channel_layout. So we read the channel_layouts using the BitrateConfig class and then provide this list as a default [AudioChannelLayout.sorted_values()[0].get_key()], now it will pick stereo by default, or, when a generic user provided bitrate config is there, it will pick the lowest of what ever the user has.

We can do the same for resolutions field as well.

Too bad this is not gonna work :(,
Because channel_layouts and resolutions are class variables and are initialized at indexing time, so even before the bitrate config is provided.

If you agree to the proposed idea above, we can monkey-patch the default of these fields in the __init__ of the PipelineConfig.
like this.

self.__class__.resolutions.default=[ # type: ignore bitrate_configuration.VideoResolution.sorted_values()[0].get_key()] self.__class__.channel_layouts.default=[ # type: ignore bitrate_configuration.AudioChannelLayout.sorted_values()[0].get_key()]

we need the # type: ignore now because since we did the cast() for these class attributes, mypy will think they are actual [RuntimeMap] while they are actually Field instances.

joeyparrish · 2021-07-30T20:40:21Z

tests/tests.js

      'audio_codecs': ['aac', 'opus'],
    };
    await startStreamer(inputConfigDict, pipelineConfigDict);
    await player.load(manifestUrl);

-    const trackList = player.getVariantTracks();
-    const audioCodecList = trackList.map(track => track.audioCodec);
+    let codecList = await getAudioAndVideoCodecs(manifestUrl)


Style nit: Add a semicolon at the end of this statement.

joeyparrish · 2021-07-30T20:42:18Z

tests/tests.js

@@ -726,17 +740,17 @@ function codecTests(manifestUrl, format) {
    const pipelineConfigDict = {
      'streaming_mode': 'vod',
      'resolutions': [],
+      'channel_layouts': ['stereo'],
      'audio_codecs': ['aac', 'opus'],
    };
    await startStreamer(inputConfigDict, pipelineConfigDict);
    await player.load(manifestUrl);


No need to call player.load() if you're using getAudioAndVideoCodecs(), which will do that internally.

joeyparrish · 2021-08-02T22:56:41Z

streamer/pipeline_configuration.py

@@ -307,6 +305,10 @@ class PipelineConfig(configuration.Base):


  def __init__(self, *args) -> None:
+    self.__class__.resolutions.default=[ # type: ignore


Since resolution and channel layout have a behavior of "use every option that doesn't upscale/upmix", perhaps the best default would be the entire list. Can you please try that and let me know how it goes? Also, please update the docstrings for those fields to say that they default to all defined values from the bitrate config.

joeyparrish · 2021-08-04T23:50:33Z

Looks like there are a couple of conflicts with the Windows PR. Can you rebase/merge and fix those up please?

mariocynicys · 2021-08-05T12:58:10Z

streamer/output_stream.py


    # The features that will be used to generate the output filename.
    self._features = {
      'language': input.language,
-      'channels': str(self.channels),
+      'channels': str(self.layout.max_channels),


Would you like having this to be 'channel_layout': self.layout.get_key() instead.
@joeyparrish

…Cannot assign to a type'

… on the wrong platform

channel as an input feature

91da441

joeyparrish requested changes Jul 21, 2021

View reviewed changes

mariocynicys added 5 commits July 28, 2021 10:15

channel_layouts, a list of channel layouts to encode

54f2e6e

updating the docstring for channel_layout input field

aef9678

printing the right values of a field when malformedfield error is raised

9bd8c7e

Revert "printing the right values of a field when malformedfield erro…

24e6cfd

…r is raised" This reverts commit 9bd8c7e.

printing the valid values of a validating type for RuntimeMapKeyValid…

0a12113

…ator

joeyparrish requested changes Jul 30, 2021

View reviewed changes

a proposal to make resolutions and channel_layouts fields non-required

77eca8c

joeyparrish requested changes Aug 2, 2021

View reviewed changes

defaulting to the whole bitrate configuration lists when not set

dcf39b4

joeyparrish approved these changes Aug 4, 2021

View reviewed changes

mariocynicys added 5 commits August 5, 2021 13:41

resolving conflicts

a1db72f

editing config files

3cfbbc9

resolving conflicts?

3dede6d

we will no longer use this module

097447c

much expressive examples

a245b78

mariocynicys commented Aug 5, 2021

View reviewed changes

mariocynicys added 6 commits August 6, 2021 06:03

Merge branch 'master' into channel-as-input-feature

fe4fbb8

using the windows monkey-patch for subprocess

92b9f03

type: ignore because 'streamer\subprocessWindowsPatch.py:100: error: …

805bf2b

…Cannot assign to a type'

addressing a tiny TODO

d671575

using sys.platform so mypy doesn't check platform specific code areas…

07dd21d

… on the wrong platform

minor edit

ac439e8

joeyparrish approved these changes Aug 10, 2021

View reviewed changes

joeyparrish merged commit 5a5b53d into shaka-project:master Aug 10, 2021

mariocynicys deleted the channel-as-input-feature branch August 10, 2021 04:05

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Channel count as an input feature #84

Channel count as an input feature #84

mariocynicys commented Jul 21, 2021 •

edited

Loading

mariocynicys commented Jul 21, 2021

joeyparrish Jul 21, 2021

mariocynicys Jul 21, 2021

mariocynicys Jul 21, 2021

joeyparrish Jul 21, 2021

joeyparrish Jul 30, 2021 •

edited

Loading

mariocynicys Jul 31, 2021

joeyparrish Jul 30, 2021

mariocynicys Jul 31, 2021

mariocynicys Jul 31, 2021

mariocynicys Jul 31, 2021

joeyparrish Jul 30, 2021

joeyparrish Jul 30, 2021

joeyparrish Aug 2, 2021

joeyparrish commented Aug 4, 2021

mariocynicys Aug 5, 2021

		@@ -307,6 +305,10 @@ class PipelineConfig(configuration.Base):


		def __init__(self, *args) -> None:
		self.__class__.resolutions.default=[ # type: ignore

Channel count as an input feature #84

Channel count as an input feature #84

Conversation

mariocynicys commented Jul 21, 2021 • edited Loading

mariocynicys commented Jul 21, 2021

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

joeyparrish Jul 30, 2021 • 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

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

joeyparrish commented Aug 4, 2021

Choose a reason for hiding this comment

mariocynicys commented Jul 21, 2021 •

edited

Loading

joeyparrish Jul 30, 2021 •

edited

Loading