[FFmpeg-devel] [PATCH 7/10] GXF Encoder Fixes and HD support (patch broken up)
Reuben Martin
reuben.m
Fri Oct 8 04:06:30 CEST 2010
Yo, back on Thursday, October 07, 2010 Baptiste Coudurier was all like:
> Hi,
>
> On 09/20/2010 05:06 PM, Reuben Martin wrote:
> > Yo, back on Monday, September 13, 2010 Reuben Martin was all like:
> >> Yo, back on Monday, September 13, 2010 Baptiste Coudurier was all like:
> >>> 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.
> >>>>
> >>>>
> >>>> --- 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.
> >>>
> >>
> >> Pardon my in-experience with this but how do I make it so that the patch applies to both the trunk and to the trunk + patch #4?
> >>
> >> Or do I even need to worry about that?
> >>
> >> I updated the patch with doxy and got rid of elements from patch #4. (i.e. will not apply correctly over-top of patch #4. If this is not what you meant, please let me know.)
> >>
> >>
> >>> Patch ok except these.
> >>>
> >>>
> >>
> > Updated again to include regression test diff.regression diff is for applying this patch individually to trunk. Regression test diff will be different if applied on top of patches #01 and #03)
> >
>
> Previous patch has been applied.
>
> > [...]
> >
> > @@ -493,11 +502,17 @@
> >
> > static int gxf_write_umf_media_dv(ByteIOContext *pb, GXFStreamContext *sc)
> > {
> > - int i;
> > -
> > - for (i = 0; i< 8; i++) {
> > - put_be32(pb, 0);
> > - }
> > + if (sc->aspect_ratio)
> > + put_le32(pb, 0x0080); /* 16:9 */
> > + else
> > + put_le32(pb, 0x0040); /* 4:3 */
> > + put_le32(pb, 0);
> > + put_le32(pb, 0);
> > + put_le32(pb, 0);
> > + put_le32(pb, 0);
> > + put_le32(pb, 0);
> > + put_le32(pb, 0);
> > + put_le32(pb, 0);
> > return 32;
>
> DV auxiliary information must be modified as well.
eeek. How did I miss that? Updated to write the DV aux info.
Note that I also added a check to set DV type in the aux info to dvcam if the yuv420p color space is used for DV25 content. Otherwise it is left as dvcpro.
Again there are some assumptions made here since I don't have access to the spec docs:
- I'm not sure if there is a special aux info id similar to how mpeg aux info has it's own special id. The generic aux info id seems to work fine. The tstream parser I'm using doesn't seem to care what the value for the aux info id is set to, as long as the data chunk is the correct length that it is expecting.
- I'm not sure on the specifics of which bit is to be set for the DV25 aux info when setting the type to DVCAM. This patch sets the first bit, but the tstream parser identifies it as DVCAM if any of the first 4 bits are turned on.
- For the DVCPROHD video streams, I guess it is just assumed that it is 16:9? I will have to look into this more, but of the few samples I have from GrassValley no aux info is shown by tstream for any of the DVPROHD video streams. This does not matter for this particular patch though, that would be something to add to patch #8 if needed.
>
> > [...]
> >
> > +
> > + if (fabs(av_q2d(display_aspect_ratio))>= 1.777)
> > + sc->aspect_ratio = 1;
> > + else
> > + sc->aspect_ratio = 0;
>
> Can be simplified: sc->aspect_ratio = fabs...
Done.
Regression test values also updated.
-Reuben
>
> [...]
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 07-gxf__set_DAR.patch
Type: text/x-patch
Size: 5080 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101007/fd53d603/attachment.bin>
More information about the ffmpeg-devel
mailing list