| Message ID | 20190828151250.12810-1-villastar@yahoo.com.au |
|---|---|
| State | Superseded |
| Headers | show |
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@live.com.au> > Co-Authored-By: Kah Goh <villastar@yahoo.com.au> > Signed-off-by: Kah Goh <villastar@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 [...]
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@live.com.au> > > Co-Authored-By: Kah Goh <villastar@yahoo.com.au> > > Signed-off-by: Kah Goh <villastar@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. > > 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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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@live.com.au> > > > Co-Authored-By: Kah Goh <villastar@yahoo.com.au> > > > Signed-off-by: Kah Goh <villastar@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@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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; + } return rfc4175_finalize_packet(data, pkt, st->index); } else if (missed_last_packet) { return 0; @@ -232,5 +266,6 @@ const RTPDynamicProtocolHandler ff_rfc4175_rtp_handler = { .codec_id = AV_CODEC_ID_BITPACKED, .priv_data_size = sizeof(PayloadContext), .parse_sdp_a_line = rfc4175_parse_sdp_line, + .init = rfc4175_initialize, .parse_packet = rfc4175_handle_packet, };
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@live.com.au> Co-Authored-By: Kah Goh <villastar@yahoo.com.au> Signed-off-by: Kah Goh <villastar@yahoo.com.au> --- libavformat/rtpdec_rfc4175.c | 41 +++++++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 3 deletions(-)