[FFmpeg-devel] [PATCH 5/5] mpegtsenc: handle audio channel specific language metadata

[Janne Grunau]

On Thu, Feb 17, 2011 at 01:05:53AM +0200, Anssi Hannula wrote:
> On 17.02.2011 00:43, Janne Grunau wrote:
> > On Mon, Feb 14, 2011 at 08:43:42PM +0200, Anssi Hannula wrote:
> >> ---
> >>  libavformat/mpegtsenc.c |   29 ++++++++++++++++++++++++++++-
> >>  1 files changed, 28 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> >> index 6945b4b..dac0e4e 100644
> >> --- a/libavformat/mpegtsenc.c
> >> +++ b/libavformat/mpegtsenc.c
> >> @@ -283,7 +283,33 @@ static void mpegts_write_pmt(AVFormatContext *s, MpegTSService *service)
> >>          /* write optional descriptors here */
> >>          switch(st->codec->codec_type) {
> >>          case AVMEDIA_TYPE_AUDIO:
> >> -            if (lang && strlen(lang->value) == 3) {
> >> +            {
> > 
> > why? I see that AVMEDIA_TYPE_SUBTITLE has it too but it is not requirred
> > and it makes this patch much harder to read.
> 
> Due to the below ch_lang declaration?

ah, missed that. a av_metadata_has_key() might be useful for this case.
 
> >> +                AVMetadataTag *ch_lang = av_metadata_get(st->metadata, "channel0/language", NULL, 0);
> >> +                if (ch_lang && strlen(ch_lang->value) == 3) {
> >> +                    /* add channel specific tags */
> >> +                    uint8_t* lang_len_ptr;
> >> +                    int ch;
> >> +                    char *language_key = av_strdup("channel0/language");
> >> +                    if (!language_key)
> >> +                        break;
> >> +                    *q++ = 0x0a; /* ISO 639 language descriptor */
> >> +                    lang_len_ptr = q++;
> >> +                    for (ch = 0; ch <= 9; ch++) {
> >> +                        if (ch > 0) {
> >> +                            language_key[7] = '0' + ch;
> >> +                            ch_lang = av_metadata_get(st->metadata, language_key, NULL, 0);
> >> +                            if (!ch_lang || strlen(ch_lang->value) != 3)
> >> +                                break;
> >> +                        }
> >> +                        *q++ = ch_lang->value[0];
> >> +                        *q++ = ch_lang->value[1];
> >> +                        *q++ = ch_lang->value[2];
> >> +                        *q++ = 0; /* undefined type */
> >> +                    }
> >> +                    *lang_len_ptr = ch * 4;
> >> +                    av_freep(&language_key);
> >> +                }
> >> +            else if (lang && strlen(lang->value) == 3) {
> > 
> > wrongly indented
> 
> I was told before to not reindent existing lines even if I change them
> (adding 'else' in this case).

hmm, don't know if that was intended. Anyway we want to less strict
about mixing code and indentation change in a single commit.

The original intention of the rule was to make patches easier to read,
the patch would have been much easier to read if the 'else if' had the
same indentation level as the 'if'.

Janne