[FFmpeg-devel] [PATCH v2] avformat/rtpdec_rfc4175: support non-zero based line numbers
Kah Goh
villastar at yahoo.com.au
Tue Sep 24 17:30:38 EEST 2019
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.
>
> 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".
More information about the ffmpeg-devel
mailing list