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

Use the ISO639 Language Descriptor when available on the input #518

Closed

Conversation

sammirata
Copy link
Contributor

When input is MPEG TS, the language information for audio streams could be available in PMT (ISO639 Language Descriptor).
This commit extracts this information and uses it when needed.

This code was submitted by the github user fpgamaster. All I did was create the pull request.

When input is MPEG TS, the language information for audio streams could be available in PMT (ISO639 Language Descriptor).
This commit extracts this information and uses it when needed.

This code was submitted by the github user fpgamaster. All I did was create the pull request.
Copy link
Contributor

@kqyang kqyang left a comment

Choose a reason for hiding this comment

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

@sammirata Thanks for working on it! I have left some comments, mostly on coding style. We generally follow Chromium C++ style guide (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md), which is based on Google C++ style guide (https://google.github.io/styleguide/cppguide.html).

Let me know if you have any questions. Appreciated your help!

@@ -80,11 +80,13 @@ static bool LookForSyncWord(const uint8_t* raw_es,

EsParserAudio::EsParserAudio(uint32_t pid,
TsStreamType stream_type,
std::string lang,
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass by reference, i.e. const std::string& lang

@@ -29,6 +29,7 @@ class EsParserAudio : public EsParser {
public:
EsParserAudio(uint32_t pid,
TsStreamType stream_type,
std::string lang,
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -51,7 +51,7 @@ class Mp2tMediaParser : public MediaParser {
// Possible values for |media_type| are defined in:
// ISO-13818.1 / ITU H.222 Table 2.34 "Media type assignments".
// |pes_pid| is part of the Program Map Table refered by |pmt_pid|.
void RegisterPes(int pmt_pid, int pes_pid, int media_type);
void RegisterPes(int pmt_pid, int pes_pid, int media_type, std::string lang);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -273,10 +273,12 @@ void Mp2tMediaParser::RegisterPmt(int program_number, int pmt_pid) {

void Mp2tMediaParser::RegisterPes(int pmt_pid,
int pes_pid,
int stream_type) {
int stream_type,
std::string lang) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

RCHECK(bit_reader->ReadBits(8, &descriptor_tag));
RCHECK(bit_reader->ReadBits(8, &descriptor_length));

if (descriptor_tag == 10) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Define a constant for '10':

const int kISO639LanguageDescriptor = 10;

@@ -18,7 +18,7 @@ class TsSectionPmt : public TsSectionPsi {
// RegisterPesCb::Run(int pes_pid, int stream_type);
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment needs to be updated

}

es_info_length -= 2 + descriptor_length;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add instructions to validate that there are no corruptions in ES info descriptors and skip the additional data if there is.

RCHECK(es_info_length >= 0);
RCHECK(bit_reader->SkipBytes(es_info_length);

RCHECK(bit_reader->ReadBits(8, &lang[1]));
RCHECK(bit_reader->ReadBits(8, &lang[2]));
RCHECK(bit_reader->SkipBits(8)); // audio_type

Copy link
Contributor

Choose a reason for hiding this comment

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

Break to avoid unnecessary processing:

es_info_length -= 2 + 4;
break;

char lang[] = { 'u', 'n', 'd', '\0' };

while (es_info_length) {
RCHECK(bit_reader->ReadBits(8, &descriptor_tag));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace tab with spaces. Same elsewhere. We do not use tab in this project as it does not render consistently on different systems.

You can check the format using packager/tools/git/check_formatting.py:

$ packager/tools/git/check_formatting.py <base>
is the commit before your change. An example is HEAD^.

@@ -76,6 +76,7 @@ bool TsSectionPmt::ParsePsiSection(BitReader* bit_reader) {
// (4 bytes = size of the CRC).
int pid_map_end_marker = section_start_marker - section_length + 4;
std::map<int, int> pid_map;
std::map<int, std::string> pid_lang;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd prefer defining a structure for it, for example PesCbInfo,

struct PesCbInfo {
  int stream_type;
  std::string lang;
};

@joeyparrish
Copy link
Member

Closing due to lack of response.

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants