| Message ID | 20f1aa81-c9e0-e0c5-3653-2627d1712a18@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Feb 15, 2018 at 4:12 AM Stefan Pöschel <basic.master@gmx.de> wrote: > The supplementary audio descriptor is defined in ETSI EN 300 468 and > provides more details regarding accessibility audio tracks, especially > the normative annex J contains a detailed description of its use. > > Its language code (if present) overrides the language code of an also > present ISO 639 language descriptor. > > Note that this also changes the priority of multiple descriptors with > language from "the last descriptor with language within the ES loop" to > "specific descriptor over general ISO 639 descriptor". > --- > libavformat/mpegts.c | 37 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > index 0a3ad05..e5d0e1e 100644 > --- a/libavformat/mpegts.c > +++ b/libavformat/mpegts.c > @@ -1840,7 +1840,9 @@ int ff_parse_mpeg2_descriptor(AVFormatContext *fc, > AVStream *st, int stream_type > } > if (i && language[0]) { > language[i - 1] = 0; > - av_dict_set(&st->metadata, "language", language, 0); > + /* don't overwrite language, as it may already have been set > by > + * another, more specific descriptor (e.g. supplementary > audio) */ > + av_dict_set(&st->metadata, "language", language, > AV_DICT_DONT_OVERWRITE); > } > break; > case 0x05: /* registration descriptor */ > @@ -1895,6 +1897,39 @@ int ff_parse_mpeg2_descriptor(AVFormatContext *fc, > AVStream *st, int stream_type > st->internal->need_context_update = 1; > } > } > + if (ext_desc_tag == 0x06) { /* supplementary audio descriptor */ > + char flags; > + > + if (desc_len < 1) > + return AVERROR_INVALIDDATA; > + flags = get8(pp, desc_end); > + > + switch ((flags >> 2) & 0x1F) { > + case 0x01: > + st->disposition |= AV_DISPOSITION_VISUAL_IMPAIRED; > + break; > + case 0x02: > + st->disposition |= AV_DISPOSITION_HEARING_IMPAIRED; > + break; > + case 0x03: > + st->disposition |= AV_DISPOSITION_VISUAL_IMPAIRED; > + break; > + } > + > + if(flags & 0x01) { > + if (desc_len < 4) > + return AVERROR_INVALIDDATA; > + language[0] = get8(pp, desc_end); > + language[1] = get8(pp, desc_end); > + language[2] = get8(pp, desc_end); > + language[3] = 0; > + > + /* This language always has to override a possible > + * ISO 639 language descriptor language */ > + if(language[0]) > + av_dict_set(&st->metadata, "language", language, 0); > + } > + } > break; > default: > break; Patch looks reasonable to me. It might also be worth surfacing when the audio descriptor flags contain mix_type=0, as this indicates a dependent stream which cannot be played standalone. Aman > -- > 2.7.4 > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
Am 15.02.2018 um 20:52 schrieb Aman Gupta: > Patch looks reasonable to me. Great! > It might also be worth surfacing when the audio descriptor flags contain > mix_type=0, as this indicates a dependent stream which cannot be played > standalone. Yes, this seems to be useful, as both mixing types are in real-world use by broadcasters. Though to be precise, also with mix_type 0, the stream can be played standalone. It just will contain only the audio description itself, not mixed with the original soundtrack. I would however welcome if adding the flag could be done by someone else, as I don't have deep knowledge in FFmpeg's internals and any implications of adding a new field (which seems to be necessary here). Regards, Stefan
On Thu, Feb 15, 2018 at 12:33 PM, Stefan Pöschel <basic.master@gmx.de> wrote: > Am 15.02.2018 um 20:52 schrieb Aman Gupta: > > Patch looks reasonable to me. > > Great! > > > It might also be worth surfacing when the audio descriptor flags contain > > mix_type=0, as this indicates a dependent stream which cannot be played > > standalone. > > Yes, this seems to be useful, as both mixing types are in real-world use > by broadcasters. Though to be precise, also with mix_type 0, the stream > can be played standalone. It just will contain only the audio > description itself, not mixed with the original soundtrack. > > I would however welcome if adding the flag could be done by someone > else, as I don't have deep knowledge in FFmpeg's internals and any > implications of adding a new field (which seems to be necessary here). > One option would be to re-use an existing flag. AV_DISPOSITION_CLEAN_EFFECTS seems to have a similar meaning, but is already used for a different purpose in mpegts.c Another option is AV_DISPOSITION_DESCRIPTIONS which has a similar meaning as well. Or, maybe it's best to create a new AV_DISPOSITION_* for this purpose. I'm happy to write a patch for this if we can decide on whether using AV_DISPOSITION_DESCRIPTIONS makes sense, or come up with a good name for a new flag. Thanks, Aman > > Regards, > Stefan > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
Am 16.02.2018 um 05:10 schrieb Aman Gupta: > One option would be to re-use an existing flag. > > AV_DISPOSITION_CLEAN_EFFECTS seems to have a similar meaning, but is > already used for a different purpose in mpegts.c Right, so this would not be a distinct indicator. > Another option is AV_DISPOSITION_DESCRIPTIONS which has a similar meaning > as well. Mhh...I think this would rather mean to re-use this flag for a different meaning of a different track type. IMO this would rather lead to confusion. > Or, maybe it's best to create a new AV_DISPOSITION_* for this purpose. Yes, this seems to be the best option. It should be as generic as possible for any possible re-use in other use cases. What about AV_DISPOSITION_DEPENDENT? > I'm happy to write a patch for this if we can decide on whether using > AV_DISPOSITION_DESCRIPTIONS > makes sense, or come up with > a good name for a new flag. Great! Regards, Stefan
On Thu, Feb 15, 2018 at 4:11 AM, Stefan Pöschel <basic.master@gmx.de> wrote: > The supplementary audio descriptor is defined in ETSI EN 300 468 and > provides more details regarding accessibility audio tracks, especially > the normative annex J contains a detailed description of its use. > > Its language code (if present) overrides the language code of an also > present ISO 639 language descriptor. > > Note that this also changes the priority of multiple descriptors with > language from "the last descriptor with language within the ES loop" to > "specific descriptor over general ISO 639 descriptor". > --- > libavformat/mpegts.c | 37 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > index 0a3ad05..e5d0e1e 100644 > --- a/libavformat/mpegts.c > +++ b/libavformat/mpegts.c > @@ -1840,7 +1840,9 @@ int ff_parse_mpeg2_descriptor(AVFormatContext *fc, > AVStream *st, int stream_type > } > if (i && language[0]) { > language[i - 1] = 0; > - av_dict_set(&st->metadata, "language", language, 0); > + /* don't overwrite language, as it may already have been set > by > + * another, more specific descriptor (e.g. supplementary > audio) */ > + av_dict_set(&st->metadata, "language", language, > AV_DICT_DONT_OVERWRITE); > } > break; > case 0x05: /* registration descriptor */ > @@ -1895,6 +1897,39 @@ int ff_parse_mpeg2_descriptor(AVFormatContext *fc, > AVStream *st, int stream_type > st->internal->need_context_update = 1; > } > } > + if (ext_desc_tag == 0x06) { /* supplementary audio descriptor */ > + char flags; > + > + if (desc_len < 1) > + return AVERROR_INVALIDDATA; > + flags = get8(pp, desc_end); > + > + switch ((flags >> 2) & 0x1F) { > + case 0x01: > + st->disposition |= AV_DISPOSITION_VISUAL_IMPAIRED; > + break; > + case 0x02: > + st->disposition |= AV_DISPOSITION_HEARING_IMPAIRED; > + break; > + case 0x03: > + st->disposition |= AV_DISPOSITION_VISUAL_IMPAIRED; > + break; > + } > + > + if(flags & 0x01) { > Please add a space after "if" here. > + if (desc_len < 4) > + return AVERROR_INVALIDDATA; > + language[0] = get8(pp, desc_end); > + language[1] = get8(pp, desc_end); > + language[2] = get8(pp, desc_end); > + language[3] = 0; > + > + /* This language always has to override a possible > + * ISO 639 language descriptor language */ > + if(language[0]) > Same, missing space. > + av_dict_set(&st->metadata, "language", language, 0); > + } > + } > break; > default: > break; > -- > 2.7.4 > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
Am 16.02.2018 um 20:01 schrieb Aman Gupta: >> + if(flags & 0x01) { > > Please add a space after "if" here. > >> + if(language[0]) > > Same, missing space. OK, I will add the spaces. And also add the respective field names as comments, as you did for the mix_type.
diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c index 0a3ad05..e5d0e1e 100644 --- a/libavformat/mpegts.c +++ b/libavformat/mpegts.c @@ -1840,7 +1840,9 @@ int ff_parse_mpeg2_descriptor(AVFormatContext *fc, AVStream *st, int stream_type } if (i && language[0]) { language[i - 1] = 0; - av_dict_set(&st->metadata, "language", language, 0); + /* don't overwrite language, as it may already have been set by + * another, more specific descriptor (e.g. supplementary audio) */ + av_dict_set(&st->metadata, "language", language, AV_DICT_DONT_OVERWRITE); } break; case 0x05: /* registration descriptor */ @@ -1895,6 +1897,39 @@ int ff_parse_mpeg2_descriptor(AVFormatContext *fc, AVStream *st, int stream_type st->internal->need_context_update = 1; } } + if (ext_desc_tag == 0x06) { /* supplementary audio descriptor */ + char flags; + + if (desc_len < 1) + return AVERROR_INVALIDDATA; + flags = get8(pp, desc_end); + + switch ((flags >> 2) & 0x1F) { + case 0x01: + st->disposition |= AV_DISPOSITION_VISUAL_IMPAIRED; + break; + case 0x02: + st->disposition |= AV_DISPOSITION_HEARING_IMPAIRED; + break; + case 0x03: + st->disposition |= AV_DISPOSITION_VISUAL_IMPAIRED; + break; + } + + if(flags & 0x01) { + if (desc_len < 4) + return AVERROR_INVALIDDATA; + language[0] = get8(pp, desc_end); + language[1] = get8(pp, desc_end); + language[2] = get8(pp, desc_end); + language[3] = 0; + + /* This language always has to override a possible + * ISO 639 language descriptor language */ + if(language[0]) + av_dict_set(&st->metadata, "language", language, 0); + } + } break; default: break;