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

[Michael Niedermayer]

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

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190911/e4561042/attachment.sig>