[FFmpeg-devel] [PATCH 7/10] GXF Encoder Fixes and HD support (patch broken up)

[Baptiste Coudurier]

On 09/13/2010 04:31 PM, Reuben Martin wrote:
> Yo, back on Thursday, September 09, 2010 Baptiste Coudurier was all like:
>> On 09/08/2010 06:39 PM, Reuben Martin wrote:
>>> Yo, back on Wednesday, September 08, 2010 Reuben Martin was all like:
>>>>
>>>> 07-gxf__set_DAR.patch
>>>>
>>>> FEATURE: Adds support for storing the video content's display aspect ration in the GXF file. This is particularly important with 16:9 NTSC where it is rather difficult to deduce the display aspect ratio simply from the frame size. Also useful for differentiating beween 1920x1080 and 1440x1080 since the main header on indicates line-height.
>>>>
>>>>
>>>> 07-gxf__set_DAR.patch
>>>>
>>>>
>>>> --- ffmpeg-old/libavformat/gxfenc.c	2010-09-08 17:00:53.716000133 -0500
>>>> +++ ffmpeg-new/libavformat/gxfenc.c	2010-09-08 17:02:05.521000116 -0500
>>>> @@ -42,6 +42,7 @@
>>>>        int p_per_gop;
>>>>        int b_per_i_or_p; ///<   number of B frames per I frame or P frame
>>>>        int first_gop_closed;
>>>> +    int aspect_ratio;
>>>>        unsigned order;   ///<   interleaving order
>>>>    } GXFStreamContext;
>>>>
>>>> @@ -187,10 +188,11 @@
>>>>            starting_line = 23; // default PAL
>>>>
>>>>        size = snprintf(buffer, 1024, "Ver 1\nBr %.6f\nIpg 1\nPpi %d\nBpiop %d\n"
>>>> -                    "Pix 0\nCf %d\nCg %d\nSl %d\nnl16 %d\nVi 1\nf1 1\n",
>>>> +                    "Pix 0\nCf %d\nCg %d\nSl %d\nnl16 %d\nVi 1\nf1 1\nar %d\n",
>>>>                        (float)st->codec->bit_rate, sc->p_per_gop, sc->b_per_i_or_p,
>>>>                        st->codec->pix_fmt == PIX_FMT_YUV422P ? 2 : 1, sc->first_gop_closed == 1,
>>>> -                    starting_line, st->codec->height % 16 == 0 ? (st->codec->height / 16) : ((st->codec->height / 16) + 1 );
>>>> +                    starting_line, st->codec->height % 16 == 0 ? (st->codec->height / 16) : ((st->codec->height / 16) + 1 ),
>>>> +                    sc->aspect_ratio);
>>
>> This contains modifications from a previous patch.
>>
>>> [...]
>>>
>>>> @@ -673,6 +689,29 @@
>>>>                    av_log(s, AV_LOG_ERROR, "video stream must be the first track\n");
>>>>                    return -1;
>>>>                }
>>>> +
>>>> +            switch (st->codec->height) {
>>>> +                case 512:
>>>> +                    sans_vbi_height = 480;
>>>> +                    break;
>>>> +                case 608:
>>>> +                    sans_vbi_height = 576;
>>>> +                    break;
>>>> +                default:
>>>> +                    sans_vbi_height = st->codec->height;
>>>> +                    break;
>>>> +            }
>>
>> if (st->codec->height == 608 || st->codec->height == 512) // VBI
>>       sans_vbi_height = st->codec->height - 32;
>> else
>>       sans_vbi_height = st->codec->height;
>>
>> seems simpler to me.
>>
>>>> +            av_reduce(&display_aspect_ratio.num,&display_aspect_ratio.den,
>>>> +                st->codec->width*st->sample_aspect_ratio.num,
>>>> +                sans_vbi_height*st->sample_aspect_ratio.den,
>>>> +                1024*1024);
>>>> +
>>   >
>>   >  [...]
>>   >
>>>> +            if (display_aspect_ratio.num == 16&&   display_aspect_ratio.den == 9)
>>>> +                sc->aspect_ratio = 1;
>>>> +            else
>>>> +                sc->aspect_ratio = 0;
>>>> +
>>
>> This seems a bit harsh to me that is everything>  1.777 should be
>> considered widescreen.
>>
>>
>
> Updated. Requested changes made. Still contains (updated) changes from patch #4.
>
>
> 07-gxf__set_DAR.patch
>
>
> --- ffmpeg-old/libavformat/gxfenc.c	2010-09-08 17:00:53.716000133 -0500
> +++ ffmpeg-new/libavformat/gxfenc.c	2010-09-08 17:02:05.521000116 -0500
> @@ -42,6 +42,7 @@
>       int p_per_gop;
>       int b_per_i_or_p; ///<  number of B frames per I frame or P frame
>       int first_gop_closed;
> +    int aspect_ratio;

Add doxy mentioning that 1 means 16:9

>       unsigned order;   ///<  interleaving order
>   } GXFStreamContext;
>
> @@ -187,10 +188,11 @@
>           starting_line = 23; // default PAL
>
>       size = snprintf(buffer, 1024, "Ver 1\nBr %.6f\nIpg 1\nPpi %d\nBpiop %d\n"
> -                    "Pix 0\nCf %d\nCg %d\nSl %d\nnl16 %d\nVi 1\nf1 1\n",
> +                    "Pix 0\nCf %d\nCg %d\nSl %d\nnl16 %d\nVi 1\nf1 1\nar %d\n",
>                       (float)st->codec->bit_rate, sc->p_per_gop, sc->b_per_i_or_p,
>                       st->codec->pix_fmt == PIX_FMT_YUV422P ? 2 : 1, sc->first_gop_closed == 1,
> -                    starting_line, (st->codec->height + 15) / 16);
> +                    starting_line, (st->codec->height + 15) / 16,
> +                    sc->aspect_ratio);

Please don't mix 2 changes in one patch, it must be commited separately.

Patch ok except these.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org