| Message ID | 20190930132720.172212-1-villastar@yahoo.com.au |
|---|---|
| State | New |
| Headers | show |
On Mon, Sep 30, 2019 at 09:27:20PM +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 adds support for non-zero based line numbers and addresses > the following issues when it starts 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 | 49 +++++++++++++++++++++++++++++++++--- > 1 file changed, 45 insertions(+), 4 deletions(-) > > diff --git a/libavformat/rtpdec_rfc4175.c b/libavformat/rtpdec_rfc4175.c > index e9c62c1389..47d5d23dd6 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, > @@ -188,7 +202,7 @@ static int rfc4175_handle_packet(AVFormatContext *ctx, PayloadContext *data, > > /* and now iterate over every scan lines */ > do { > - int copy_offset; > + int copy_offset, copy_to_line; > > if (payload_len < data->pgroup) > return AVERROR_INVALIDDATA; > @@ -199,17 +213,34 @@ 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; > > if (length > payload_len) > 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) > + copy_to_line = line - data->first_line_number; > + if (copy_to_line < 0) > + /* This means the first line number we have calculated is too large, which indicates that we > + may have received some bad data. */ > return AVERROR_INVALIDDATA; > > + /* prevent ill-formed packets to write after buffer's end */ > + copy_offset = (copy_to_line * 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 +249,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 +272,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, > }; > -- > 2.23.0 > Ping
On Thu, Nov 07, 2019 at 10:27:31PM +0800, Kah Goh wrote: > On Mon, Sep 30, 2019 at 09:27:20PM +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 adds support for non-zero based line numbers and addresses > > the following issues when it starts 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 | 49 +++++++++++++++++++++++++++++++++--- > > 1 file changed, 45 insertions(+), 4 deletions(-) > > > > diff --git a/libavformat/rtpdec_rfc4175.c b/libavformat/rtpdec_rfc4175.c > > index e9c62c1389..47d5d23dd6 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, > > @@ -188,7 +202,7 @@ static int rfc4175_handle_packet(AVFormatContext *ctx, PayloadContext *data, > > > > /* and now iterate over every scan lines */ > > do { > > - int copy_offset; > > + int copy_offset, copy_to_line; > > > > if (payload_len < data->pgroup) > > return AVERROR_INVALIDDATA; > > @@ -199,17 +213,34 @@ 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; > > > > if (length > payload_len) > > 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) > > + copy_to_line = line - data->first_line_number; > > + if (copy_to_line < 0) > > + /* This means the first line number we have calculated is too large, which indicates that we > > + may have received some bad data. */ > > return AVERROR_INVALIDDATA; > > > > + /* prevent ill-formed packets to write after buffer's end */ > > + copy_offset = (copy_to_line * 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 +249,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 +272,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, > > }; > > -- > > 2.23.0 > > > > Ping I see there are multiple People working on this and others who worked previously on this. Can one of the people working on this code please review this ? Iam happy to apply it if its reviewed (assuming noone spots anything bad of course) Thanks [...]
On Thu, Nov 07, 2019 at 10:35:25PM +0100, Michael Niedermayer wrote: > On Thu, Nov 07, 2019 at 10:27:31PM +0800, Kah Goh wrote: > > On Mon, Sep 30, 2019 at 09:27:20PM +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 adds support for non-zero based line numbers and addresses > > > the following issues when it starts 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 | 49 +++++++++++++++++++++++++++++++++--- > > > 1 file changed, 45 insertions(+), 4 deletions(-) > > > > > > diff --git a/libavformat/rtpdec_rfc4175.c b/libavformat/rtpdec_rfc4175.c > > > index e9c62c1389..47d5d23dd6 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, > > > @@ -188,7 +202,7 @@ static int rfc4175_handle_packet(AVFormatContext *ctx, PayloadContext *data, > > > > > > /* and now iterate over every scan lines */ > > > do { > > > - int copy_offset; > > > + int copy_offset, copy_to_line; > > > > > > if (payload_len < data->pgroup) > > > return AVERROR_INVALIDDATA; > > > @@ -199,17 +213,34 @@ 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; > > > > > > if (length > payload_len) > > > 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) > > > + copy_to_line = line - data->first_line_number; > > > + if (copy_to_line < 0) > > > + /* This means the first line number we have calculated is too large, which indicates that we > > > + may have received some bad data. */ > > > return AVERROR_INVALIDDATA; > > > > > > + /* prevent ill-formed packets to write after buffer's end */ > > > + copy_offset = (copy_to_line * 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 +249,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 +272,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, > > > }; > > > -- > > > 2.23.0 > > > > > > > Ping > > I see there are multiple People working on this and others who worked > previously on this. Can one of the people working on this code > please review this ? > Iam happy to apply it if its reviewed (assuming noone spots anything > bad of course) Anyone? > > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > He who knows, does not speak. He who speaks, does not know. -- Lao Tsu > _______________________________________________ > 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 Thu, Nov 07, 2019 at 10:35:25PM +0100, Michael Niedermayer wrote: > On Thu, Nov 07, 2019 at 10:27:31PM +0800, Kah Goh wrote: > > On Mon, Sep 30, 2019 at 09:27:20PM +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 adds support for non-zero based line numbers and addresses > > > the following issues when it starts 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 | 49 +++++++++++++++++++++++++++++++++--- > > > 1 file changed, 45 insertions(+), 4 deletions(-) > > > > > > diff --git a/libavformat/rtpdec_rfc4175.c b/libavformat/rtpdec_rfc4175.c > > > index e9c62c1389..47d5d23dd6 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, > > > @@ -188,7 +202,7 @@ static int rfc4175_handle_packet(AVFormatContext *ctx, PayloadContext *data, > > > > > > /* and now iterate over every scan lines */ > > > do { > > > - int copy_offset; > > > + int copy_offset, copy_to_line; > > > > > > if (payload_len < data->pgroup) > > > return AVERROR_INVALIDDATA; > > > @@ -199,17 +213,34 @@ 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; > > > > > > if (length > payload_len) > > > 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) > > > + copy_to_line = line - data->first_line_number; > > > + if (copy_to_line < 0) > > > + /* This means the first line number we have calculated is too large, which indicates that we > > > + may have received some bad data. */ > > > return AVERROR_INVALIDDATA; > > > > > > + /* prevent ill-formed packets to write after buffer's end */ > > > + copy_offset = (copy_to_line * 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 +249,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 +272,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, > > > }; > > > -- > > > 2.23.0 > > > > > > > Ping > > I see there are multiple People working on this and others who worked > previously on this. Can one of the people working on this code > please review this ? > Iam happy to apply it if its reviewed (assuming noone spots anything > bad of course) > Ping > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > He who knows, does not speak. He who speaks, does not know. -- Lao Tsu > _______________________________________________ > 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".
Ping those people directly instead. On 11/7/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Nov 07, 2019 at 10:27:31PM +0800, Kah Goh wrote: >> On Mon, Sep 30, 2019 at 09:27:20PM +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 adds support for non-zero based line numbers and addresses >> > the following issues when it starts 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 | 49 +++++++++++++++++++++++++++++++++--- >> > 1 file changed, 45 insertions(+), 4 deletions(-) >> > >> > diff --git a/libavformat/rtpdec_rfc4175.c >> > b/libavformat/rtpdec_rfc4175.c >> > index e9c62c1389..47d5d23dd6 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, >> > @@ -188,7 +202,7 @@ static int rfc4175_handle_packet(AVFormatContext >> > *ctx, PayloadContext *data, >> > >> > /* and now iterate over every scan lines */ >> > do { >> > - int copy_offset; >> > + int copy_offset, copy_to_line; >> > >> > if (payload_len < data->pgroup) >> > return AVERROR_INVALIDDATA; >> > @@ -199,17 +213,34 @@ 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; >> > >> > if (length > payload_len) >> > 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) >> > + copy_to_line = line - data->first_line_number; >> > + if (copy_to_line < 0) >> > + /* This means the first line number we have calculated is >> > too large, which indicates that we >> > + may have received some bad data. */ >> > return AVERROR_INVALIDDATA; >> > >> > + /* prevent ill-formed packets to write after buffer's end */ >> > + copy_offset = (copy_to_line * 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 +249,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 +272,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, >> > }; >> > -- >> > 2.23.0 >> > >> >> Ping > > I see there are multiple People working on this and others who worked > previously on this. Can one of the people working on this code > please review this ? > Iam happy to apply it if its reviewed (assuming noone spots anything > bad of course) > > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > He who knows, does not speak. He who speaks, does not know. -- Lao Tsu >
On 11/7/19, Michael Niedermayer <michael at niedermayer.cc> wrote: > I see there are multiple People working on this and others who worked > previously on this. Can one of the people working on this code > please review this ? > Iam happy to apply it if its reviewed (assuming noone spots anything > bad of course) Just had a look at the changes and they look good to me. Jacob
diff --git a/libavformat/rtpdec_rfc4175.c b/libavformat/rtpdec_rfc4175.c index e9c62c1389..47d5d23dd6 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, @@ -188,7 +202,7 @@ static int rfc4175_handle_packet(AVFormatContext *ctx, PayloadContext *data, /* and now iterate over every scan lines */ do { - int copy_offset; + int copy_offset, copy_to_line; if (payload_len < data->pgroup) return AVERROR_INVALIDDATA; @@ -199,17 +213,34 @@ 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; if (length > payload_len) 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) + copy_to_line = line - data->first_line_number; + if (copy_to_line < 0) + /* This means the first line number we have calculated is too large, which indicates that we + may have received some bad data. */ return AVERROR_INVALIDDATA; + /* prevent ill-formed packets to write after buffer's end */ + copy_offset = (copy_to_line * 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 +249,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 +272,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 adds support for non-zero based line numbers and addresses the following issues when it starts 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 | 49 +++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 4 deletions(-)