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

[Anssi Hannula]

On 17.02.2011 01:38, Janne Grunau wrote:
> 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.

Hm? Wouldn't it be exactly the same as av_metadata_get() but with a
different return value? :)

>>>> +                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'.

OK.

-- 
Anssi Hannula