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.

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

Feat/multiple track audios #17

Merged
merged 7 commits into from
Jan 16, 2024

Conversation

luiscovelo
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@mauricioabreu mauricioabreu left a comment

Choose a reason for hiding this comment

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

Good job so far.

)

if m.WithAudio {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most of the mosaics I have ever seen in my entire life have only the audio of the first video.
Maybe we need this option to have multiple choices:

  • No audio (no_audio)
  • First input only (first_input)
  • All inputs (all_inputs)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, right!

Theses options can be a new struct, like as "Audio"?

Example:

type Audio struct {
	NoAudio    bool `json:"no_audio"`
	FirstInput bool `json:"first_input"`
	AllInputs  bool `json:"all_inputs"`
}

or

type Audio struct {
	Off        bool `json:"off"`
	FirstInput bool `json:"first_input"`
	AllInputs  bool `json:"all_inputs"`
}

What's your option about this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The field could be an enum with these 3 options.
Imagine you are creating a mosaic through an admin. This admin could provide a dropdown with these 3 options

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right,

Like this:

type Audio string

const (
	NoAudio    string = "no_audio"
	FirstInput string = "first_input"
	AllInputs  string = "all_inputs"
)

type (
	Position struct {
		X int
		Y int
	}
	Media struct {
		URL      string `json:"url"`
		Position Position
		Scale    string
	}

	Mosaic struct {
		Name          string  `json:"name"`
		BackgroundURL string  `json:"background_url"`
		Medias        []Media `json:"medias"`
		Audio         Audio   `json:"audio"`
	}
)

Using the Audio as a type, we can to assign three methods to validate audio options, such as: IsNoAudio(), IsFirstInput() or IsAllInputs().

type Audio string

func (a Audio) IsNoAudio() bool {
	return a == Audio(NoAudio)
}

func (a Audio) IsFirstInput() bool {
	return a == Audio(FirstInput)
}

func (a Audio) IsAllInputs() bool {
	return a == Audio(AllInputs)
}

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the constant approach and the Audio type, pretty cool!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good, i'll fixed it.

@luiscovelo
Copy link
Collaborator Author

@mauricioabreu,

Based in our conversations, i fixed to use a Audio as a enum type.

@mauricioabreu
Copy link
Collaborator

@luiscovelo good! How did it look like in the player? Did it play smooth with the multiple audios option?

@luiscovelo
Copy link
Collaborator Author

luiscovelo commented Jan 15, 2024

@mauricioabreu,

I tested using VLC player, I had to download a mosaic content from bucket and to import on VLC.

I'll to add this options in the HLS player into another PR, I need to evaluate in how to make in HLS.js library, but in VLC player worked very well.

Copy link
Collaborator

@mauricioabreu mauricioabreu left a comment

Choose a reason for hiding this comment

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

Good job!

@mauricioabreu mauricioabreu merged commit 95d5fcc into learn-video:main Jan 16, 2024
1 check passed
@luiscovelo luiscovelo deleted the feat/multiple-track-audios branch January 18, 2024 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants