[FFmpeg-devel] [PATCH v2] avformat/rtpdec_rfc4175: support non-zero based line numbers

Kah Goh villastar at yahoo.com.au
Tue Oct 1 15:53:08 EEST 2019


On Tue, Sep 24, 2019 at 10:30:38PM +0800, Kah Goh wrote:
> On Wed, Sep 11, 2019 at 08:28:09PM +0200, Michael Niedermayer wrote:
> > On Wed, Aug 28, 2019 at 11:12:51PM +0800, Kah Goh wrote:
> > > There are differing standards that define different starting line
> > > numbers. For example, VSF TR-03 says the line numbers starts at 1,
> > > whereas SMPTE 2110-20 says it should start at 0.
> > > 
> > > This change fixes the following issues when the line numbering start
> > >  at 1:
> > > - The first scan line was being incorrectly interpreted as the second
> > >   scan line. This means the first line in the frame was never being
> > >   populated.
> > > - The last packet for the video frame would be treated as invalid
> > >   because it would have been copied outside of the frame. Consequently,
> > >   the packet would never be "finalized" and the next packet triggers a
> > >   missed RTP marker ("Missed previous RTP marker" would keep being
> > >   logged).
> > > 
> > > VSF TR-03: http://www.videoservicesforum.org/download/technical_recommendations/VSF_TR-03_2015-11-12.pdf
> > > 
> > > Co-Authored-By: Jacob Siddall <kobe at live.com.au>
> > > Co-Authored-By: Kah Goh <villastar at yahoo.com.au>
> > > Signed-off-by: Kah Goh <villastar at yahoo.com.au>
> > > ---
> > >  libavformat/rtpdec_rfc4175.c | 41 +++++++++++++++++++++++++++++++++---
> > >  1 file changed, 38 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/libavformat/rtpdec_rfc4175.c b/libavformat/rtpdec_rfc4175.c
> > > index e9c62c1389..427d4b31e2 100644
> > > --- a/libavformat/rtpdec_rfc4175.c
> > > +++ b/libavformat/rtpdec_rfc4175.c
> > > @@ -25,6 +25,7 @@
> > >  #include "rtpdec_formats.h"
> > >  #include "libavutil/avstring.h"
> > >  #include "libavutil/pixdesc.h"
> > > +#include <stdbool.h>
> > >  
> > >  struct PayloadContext {
> > >      char *sampling;
> > > @@ -37,6 +38,12 @@ struct PayloadContext {
> > >      unsigned int pgroup; /* size of the pixel group in bytes */
> > >      unsigned int xinc;
> > >  
> > > +    /* The line number of the first line in the frame (usually either 0 or 1). */
> > > +    int first_line_number;
> > > +
> > > +    /* This is set to true once the first line number is confirmed. */
> > > +    bool first_line_number_known;
> > 
> > 
> > 
> > > +
> > >      uint32_t timestamp;
> > >  };
> > >  
> > > @@ -136,6 +143,13 @@ static int rfc4175_finalize_packet(PayloadContext *data, AVPacket *pkt,
> > >     return ret;
> > >  }
> > >  
> > > +static int rfc4175_initialize(AVFormatContext *s, int st_index, PayloadContext *data)
> > > +{
> > > +    data->first_line_number = 0;
> > > +    data->first_line_number_known = false;
> > > +    return 0;
> > > +}
> > > +
> > >  static int rfc4175_handle_packet(AVFormatContext *ctx, PayloadContext *data,
> > >                                   AVStream *st, AVPacket *pkt, uint32_t *timestamp,
> > >                                   const uint8_t * buf, int len,
> > > @@ -199,6 +213,11 @@ static int rfc4175_handle_packet(AVFormatContext *ctx, PayloadContext *data,
> > >          cont = headers[4] & 0x80;
> > >          headers += 6;
> > >  
> > > +        if (line == 0) {
> > > +            data->first_line_number = 0;
> > > +            data->first_line_number_known = true;
> > > +        }
> > > +
> > >          if (length % data->pgroup)
> > >              return AVERROR_INVALIDDATA;
> > >  
> > > @@ -206,9 +225,15 @@ static int rfc4175_handle_packet(AVFormatContext *ctx, PayloadContext *data,
> > >              length = payload_len;
> > >  
> > >          /* prevent ill-formed packets to write after buffer's end */
> > > -        copy_offset = (line * data->width + offset) * data->pgroup / data->xinc;
> > > -        if (copy_offset + length > data->frame_size)
> > > -            return AVERROR_INVALIDDATA;
> > > +        copy_offset = ((line - data->first_line_number) * data->width + offset) * data->pgroup / data->xinc;
> > > +        if (copy_offset + length > data->frame_size) {
> > > +            if (data->first_line_number_known)
> > > +                return AVERROR_INVALIDDATA;
> > > +
> > > +            // This would happen if the line numbering is 1 based. We still need to check for the RTP flag
> > > +            // marker (as per after the while loop).
> > > +            break;
> > > +        }
> > >  
> > >          dest = data->frame + copy_offset;
> > >          memcpy(dest, payload, length);
> > > @@ -218,6 +243,15 @@ static int rfc4175_handle_packet(AVFormatContext *ctx, PayloadContext *data,
> > >      } while (cont);
> > >  
> > >      if ((flags & RTP_FLAG_MARKER)) {
> > > +        if (!data->first_line_number_known) {
> > > +            data->first_line_number = line - data->height + 1;
> > > +            if (data->first_line_number < 0) {
> > > +                // This could happen if the frame does not fill up the entire height.
> > > +                data->first_line_number = 0;
> > > +                av_log(ctx, AV_LOG_WARNING, "Video frame does not fill entire height");
> > > +            }
> > > +            data->first_line_number_known = true;
> > 
> > What if the last line is larger than height ?
> > first_line_number is only checked for <0 not >1 nor >height
> > 
> > but maybe ive missed some check
> 
> The usage of the first line number in the calculation at line 228 will
> cater for cases where it is greater than 1 or the height. There is an
> assumption that the line numbering is always continuous, starting from
> the first line number going up to the first line number plus the frame
> height. 
> 
> Having said that, I just had the thought that it might be prudent to
> add an additional guards around the calculation to check for signs of
> the assumption breaking or bad data. For example, if 
> line - data->first_line_number is <0. I'll see if I can add this later
> when I get the chance.
> 

Submitted v3 of the patch that adds this check at line 227.

Link on patchwork: https://patchwork.ffmpeg.org/patch/15412/

> > 
> > thx
> > 
> > [...]
> > -- 
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> > 
> > The bravest are surely those who have the clearest vision
> > of what is before them, glory and danger alike, and yet
> > notwithstanding go out to meet it. -- Thucydides
> 
> 
> 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".


More information about the ffmpeg-devel mailing list