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

[Reuben Martin]

Yo, back on Monday, September 13, 2010 Baptiste Coudurier was all like:
> On 9/13/10 7:38 PM, Reuben Martin wrote:
> > Yo, back on Monday, September 13, 2010 Baptiste Coudurier was all like:
> >> Hi,
> >>
> >> On 09/13/2010 04:23 PM, Reuben Martin wrote:
> >>> Yo, back on Thursday, September 09, 2010 Baptiste Coudurier was all like:
> >>>> Hi,
> >>>>
> >>>> On 09/08/2010 06:34 PM, Reuben Martin wrote:
> >>>>> Yo, back on Wednesday, September 08, 2010 Reuben Martin was all like:
> >>>>>> Here's the breakdown:
> >>>>>>
> >>>>>> 01-gxf__flt_offset_error.patch
> >>>>>>
> >>>>>> BUG: The offset values for the FLT packets were getting attached to the packet preceding the packet they were supposed to be attached to. This was causing all kinds of playback and seeking issues on GrassValley's Turbo.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> 01-gxf__flt_offset_error.patch
> >>>>>>
> >>>>>>
> >>>>>> --- ffmpeg-old/libavformat/gxfenc.c	2010-09-04 11:41:27.000000000 -0500
> >>>>>> +++ ffmpeg-new/libavformat/gxfenc.c	2010-09-08 16:39:20.405000084 -0500
> >>>>>> @@ -878,7 +878,11 @@
> >>>>>>                     return -1;
> >>>>>>                 }
> >>>>>>             }
> >>>>>> -        gxf->flt_entries[gxf->flt_entries_nb++] = url_ftell(pb) / 1024;
> >>>>>> +
> >>>>>> +        if (!(gxf->flt_entries_nb))
> >>>>>> +            gxf->flt_entries[gxf->flt_entries_nb] = 4; /* First offset seems to be pretty close to this. May vary. */
> >>>>>> +        gxf->flt_entries_nb++;
> >>>>>> +        gxf->flt_entries[gxf->flt_entries_nb] = url_ftell(pb) / 1024;
> >>>>>>             gxf->nb_fields += 2; // count fields
> >>>>>>         }
> >>>>>>
> >>>>
> >>>> Looks hackish to me.
> >>>>
> >>>>
> >>>
> >>> Changes made:
> >>>
> >>> Instead of checking the offset after writing the packet and using that values as the next FLT's offset value, it checks the offset before writing the packet and uses the value as the current FLT's offset.
> >>>
> >>> This fixes the bug as before, while also correctly calculating the proper offset for FLT[0]. The previous solution assumed 4k for FLT[0] which I found to be incorrect in cases where the first packet is not a video packet.
> >>>
> >>> Also less hackish. :)
> >>>
> >>> -Reuben
> >>>
> >>>
> >>> 01-gxf__flt_offset_error.patch
> >>>
> >>>
> >>> --- ffmpeg-old/libavformat/gxfenc.c	2010-09-04 11:41:27.000000000 -0500
> >>> +++ ffmpeg-new/libavformat/gxfenc.c	2010-09-13 18:02:43.094000094 -0500
> >>> @@ -859,7 +859,9 @@
> >>>        AVStream *st = s->streams[pkt->stream_index];
> >>>        int64_t pos = url_ftell(pb);
> >>>        int padding = 0;
> >>> +    int packet_start_offset = 0;
> >>>
> >>> +    packet_start_offset = url_ftell(pb) / 1024;
> >>>        gxf_write_packet_header(pb, PKT_MEDIA);
> >>>        if (st->codec->codec_id == CODEC_ID_MPEG2VIDEO&&   pkt->size % 4) /* MPEG-2 frames must be padded */
> >>>            padding = 4 - pkt->size % 4;
> >>> @@ -878,7 +880,8 @@
> >>>                    return -1;
> >>>                }
> >>>            }
> >>> -        gxf->flt_entries[gxf->flt_entries_nb++] = url_ftell(pb) / 1024;
> >>> +        gxf->flt_entries[gxf->flt_entries_nb] = packet_start_offset;
> >>> +        gxf->flt_entries_nb++;
> >>
> >> Looks good to me except the nb++ change which is not necessary.
> >>
> >>
> >
> > That was left over from my previous change. I've attatched updated version with the nb++ moved back the way it was before.
> >
> >
> >
> >
> > 01-gxf__flt_offset_error.patch
> >
> >
> > --- ffmpeg-old/libavformat/gxfenc.c	2010-09-04 11:41:27.000000000 -0500
> > +++ ffmpeg-new/libavformat/gxfenc.c	2010-09-13 18:02:43.094000094 -0500
> > @@ -859,7 +859,9 @@
> >       AVStream *st = s->streams[pkt->stream_index];
> >       int64_t pos = url_ftell(pb);
> >       int padding = 0;
> > +    int packet_start_offset = 0;
> >
> > +    packet_start_offset = url_ftell(pb) / 1024;
> >       gxf_write_packet_header(pb, PKT_MEDIA);
> >       if (st->codec->codec_id == CODEC_ID_MPEG2VIDEO&&  pkt->size % 4) /* MPEG-2 frames must be padded */
> >           padding = 4 - pkt->size % 4;
> > @@ -878,7 +880,7 @@
> >                   return -1;
> >               }
> >           }
> > -        gxf->flt_entries[gxf->flt_entries_nb++] = url_ftell(pb) / 1024;
> > +        gxf->flt_entries[gxf->flt_entries_nb++] = packet_start_offset;
> >           gxf->nb_fields += 2; // count fields
> >       }
> >
> 
> Patch ok, I forgot to tell you to run the regression tests, this change 
> should make them differ, attach the diff so it's easier to apply.
> 
> Thanks for the work btw.
> 
> 

I believe patches #3, #4, and #7 will differ as well. Should I run the regression tests with the patches applied individually, or applied on top of each other?

I guess I'm not really sure how you deal with these patches. Do you treat them as a set, and then apply them all at once, or do you deal with them individually?

Thanks,
-Reuben