| Message ID | DM6PR14MB2633D6F49CE76BB115C518639AFD0@DM6PR14MB2633.namprd14.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jun 27, 2019 at 06:06:22AM +0000, Jacob Siddall wrote: > The previous calculation code did not account for the fact that the > copy_offset for the start of the frame array is at index 0, yet the > scan line number from the rfc4175 RTP header starts at 1. > This caused 2 issues to appear: > - The first scan line was being copied into the array where the second > scan line should be. This caused the resulting video to have a green > line at the top of it. > - Since the packet containing the last scan line would fail the > calculation, the packet with the RTP marker would not be processed > which caused a log message saying "Missed previous RTP marker" to be > outputted for each frame. > > Signed-off-by: Jacob Siddall <kobe@live.com.au> > --- > Changes in v2: > - Don't handle packet if the line number is less than 1 > > Section 12 in the VSF technical recommendation TR-03 specifies that the > video scan line numbers should start at 1. > http://www.videoservicesforum.org/download/technical_recommendations/VSF_TR-03_2015-11-12.pdf > > Changes in v3: > - Changed the commit hash abbreviation in the patch file diff to be 10 > characters in length rather than 7. This was causing the patch file > to fail when it was applied. > > libavformat/rtpdec_rfc4175.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) The patch looks reasonable to me but i dont really know rfc4175 Ideally someone who knows rfc4175 should make the final decission to apply it maybe whoever applies it can also bump the micro version so user apps have a way to detect this line shifting change Thanks [...]
On Thu, Jun 27, 2019 at 06:06:22AM +0000, Jacob Siddall wrote: > The previous calculation code did not account for the fact that the > copy_offset for the start of the frame array is at index 0, yet the > scan line number from the rfc4175 RTP header starts at 1. > This caused 2 issues to appear: > - The first scan line was being copied into the array where the second > scan line should be. This caused the resulting video to have a green > line at the top of it. > - Since the packet containing the last scan line would fail the > calculation, the packet with the RTP marker would not be processed > which caused a log message saying "Missed previous RTP marker" to be > outputted for each frame. > > Signed-off-by: Jacob Siddall <kobe@live.com.au> > --- > Changes in v2: > - Don't handle packet if the line number is less than 1 > > Section 12 in the VSF technical recommendation TR-03 specifies that the > video scan line numbers should start at 1. > http://www.videoservicesforum.org/download/technical_recommendations/VSF_TR-03_2015-11-12.pdf > > Changes in v3: > - Changed the commit hash abbreviation in the patch file diff to be 10 > characters in length rather than 7. This was causing the patch file > to fail when it was applied. > > libavformat/rtpdec_rfc4175.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) will apply thx [...]
On Mon, 5 Aug 2019 at 17:10, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Jun 27, 2019 at 06:06:22AM +0000, Jacob Siddall wrote: > > The previous calculation code did not account for the fact that the > > copy_offset for the start of the frame array is at index 0, yet the > > scan line number from the rfc4175 RTP header starts at 1. > > This caused 2 issues to appear: > > - The first scan line was being copied into the array where the second > > scan line should be. This caused the resulting video to have a green > > line at the top of it. > > - Since the packet containing the last scan line would fail the > > calculation, the packet with the RTP marker would not be processed > > which caused a log message saying "Missed previous RTP marker" to be > > outputted for each frame. > > > > Signed-off-by: Jacob Siddall <kobe@live.com.au> > > --- > > Changes in v2: > > - Don't handle packet if the line number is less than 1 > > > > Section 12 in the VSF technical recommendation TR-03 specifies that the > > video scan line numbers should start at 1. > > > http://www.videoservicesforum.org/download/technical_recommendations/VSF_TR-03_2015-11-12.pdf > > SMPTE 2110-20 says line numbers start at 0. (Yes there are 3 different "standards" for handling the line number because of the geniuses in the broadcast industry). Kieran
On Mon, Aug 05, 2019 at 06:05:29PM +0100, Kieran Kunhya wrote: > On Mon, 5 Aug 2019 at 17:10, Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > On Thu, Jun 27, 2019 at 06:06:22AM +0000, Jacob Siddall wrote: > > > The previous calculation code did not account for the fact that the > > > copy_offset for the start of the frame array is at index 0, yet the > > > scan line number from the rfc4175 RTP header starts at 1. > > > This caused 2 issues to appear: > > > - The first scan line was being copied into the array where the second > > > scan line should be. This caused the resulting video to have a green > > > line at the top of it. > > > - Since the packet containing the last scan line would fail the > > > calculation, the packet with the RTP marker would not be processed > > > which caused a log message saying "Missed previous RTP marker" to be > > > outputted for each frame. > > > > > > Signed-off-by: Jacob Siddall <kobe@live.com.au> > > > --- > > > Changes in v2: > > > - Don't handle packet if the line number is less than 1 > > > > > > Section 12 in the VSF technical recommendation TR-03 specifies that the > > > video scan line numbers should start at 1. > > > > > http://www.videoservicesforum.org/download/technical_recommendations/VSF_TR-03_2015-11-12.pdf > > > > > SMPTE 2110-20 says line numbers start at 0. (Yes there are 3 different > "standards" for handling the line number because of the geniuses in the > broadcast industry). Jacob, can you look into this ? should i revert until this is fixed? Thanks [...]
> On Mon, Aug 05, 2019 at 06:05:29PM +0100, Kieran Kunhya wrote: > > On Mon, 5 Aug 2019 at 17:10, Michael Niedermayer <michael@niedermayer.cc> > > wrote: > > > > > On Thu, Jun 27, 2019 at 06:06:22AM +0000, Jacob Siddall wrote: > > > > The previous calculation code did not account for the fact that the > > > > copy_offset for the start of the frame array is at index 0, yet the > > > > scan line number from the rfc4175 RTP header starts at 1. > > > > This caused 2 issues to appear: > > > > - The first scan line was being copied into the array where the second > > > > scan line should be. This caused the resulting video to have a green > > > > line at the top of it. > > > > - Since the packet containing the last scan line would fail the > > > > calculation, the packet with the RTP marker would not be processed > > > > which caused a log message saying "Missed previous RTP marker" to be > > > > outputted for each frame. > > > > > > > > Signed-off-by: Jacob Siddall <kobe@live.com.au> > > > > --- > > > > Changes in v2: > > > > - Don't handle packet if the line number is less than 1 > > > > > > > > Section 12 in the VSF technical recommendation TR-03 specifies that the > > > > video scan line numbers should start at 1. > > > > > > > http://www.videoservicesforum.org/download/technical_recommendations/VSF_TR-03_2015-11-12.pdf > > > > > > > > SMPTE 2110-20 says line numbers start at 0. (Yes there are 3 different > > "standards" for handling the line number because of the geniuses in the > > broadcast industry). > > Jacob, can you look into this ? > > should i revert until this is fixed? > > Thanks Michael, yes lets revert this commit for now and come up with a better solution that can cater for both starting line number cases. I think we could handle this in 1 of 2 ways: 1. Add some sort of flag which allows overriding of the value which line numbers start from. We could leave the default as line numbering starting from 0 (how the previous code worked). If the flag is set, then the line numbering starts from 1. 2. Add code which detects what the value of the first line number is and use that value for any future processing. This could work by assuming line numbering starts at 1 until it sees a frame with line 0. This value would reside in memory so if ffmpeg is restarted, the line number detection would re-run. What are your thoughts on this? > On Mon, Aug 05, 2019 at 06:05:29PM +0100, Kieran Kunhya wrote: > SMPTE 2110-20 says line numbers start at 0. (Yes there are 3 different > "standards" for handling the line number because of the geniuses in the > broadcast industry). Kieran can you send me the pdf on SMPTE 2110-20? I can't seem to find where to access it that's not behind a paywall. Also do you know what the 3 different "standards" are? Jacob
On Tue, Aug 06, 2019 at 05:42:22AM +0000, Jacob Siddall wrote: > > On Mon, Aug 05, 2019 at 06:05:29PM +0100, Kieran Kunhya wrote: > > > On Mon, 5 Aug 2019 at 17:10, Michael Niedermayer <michael@niedermayer.cc> > > > wrote: > > > > > > > On Thu, Jun 27, 2019 at 06:06:22AM +0000, Jacob Siddall wrote: > > > > > The previous calculation code did not account for the fact that the > > > > > copy_offset for the start of the frame array is at index 0, yet the > > > > > scan line number from the rfc4175 RTP header starts at 1. > > > > > This caused 2 issues to appear: > > > > > - The first scan line was being copied into the array where the second > > > > > scan line should be. This caused the resulting video to have a green > > > > > line at the top of it. > > > > > - Since the packet containing the last scan line would fail the > > > > > calculation, the packet with the RTP marker would not be processed > > > > > which caused a log message saying "Missed previous RTP marker" to be > > > > > outputted for each frame. > > > > > > > > > > Signed-off-by: Jacob Siddall <kobe@live.com.au> > > > > > --- > > > > > Changes in v2: > > > > > - Don't handle packet if the line number is less than 1 > > > > > > > > > > Section 12 in the VSF technical recommendation TR-03 specifies that the > > > > > video scan line numbers should start at 1. > > > > > > > > > http://www.videoservicesforum.org/download/technical_recommendations/VSF_TR-03_2015-11-12.pdf > > > > > > > > > > > SMPTE 2110-20 says line numbers start at 0. (Yes there are 3 different > > > "standards" for handling the line number because of the geniuses in the > > > broadcast industry). > > > > Jacob, can you look into this ? > > > > should i revert until this is fixed? > > > > Thanks > > Michael, yes lets revert this commit for now and come up with a better > solution that can cater for both starting line number cases. ok, will revert > > I think we could handle this in 1 of 2 ways: > 1. Add some sort of flag which allows overriding of the value which line > numbers start from. > We could leave the default as line numbering starting from 0 (how the > previous code worked). If the flag is set, then the line numbering > starts from 1. > 2. Add code which detects what the value of the first line number is and > use that value for any future processing. This could work by assuming > line numbering starts at 1 until it sees a frame with line 0. This > value would reside in memory so if ffmpeg is restarted, the line > number detection would re-run. > > What are your thoughts on this? It should be automatic, not requiring user input of course. So if the format can be detected somehow thats best Next best simply detecting if the first line is 0 or 1 and keeping that in the nearest instance context probably. Any global variables to hold that state would of course not work as there can be multiple instances running at the same time thanks [...]
> > > Jacob, can you look into this ? > > > > > > should i revert until this is fixed? > > > > > > Thanks > > > > Michael, yes lets revert this commit for now and come up with a better > > solution that can cater for both starting line number cases. > > ok, will revert Michael, My friend and I have submitted a new patch which handles both cases of line numbers starting at 0 or 1. The link to the thread is http://ffmpeg.org/pipermail/ffmpeg-devel/2019-August/249055.html And the link to patchwork is https://patchwork.ffmpeg.org/patch/14753/ When you get a chance, can you have a look at the patch? Jacob
diff --git a/libavformat/rtpdec_rfc4175.c b/libavformat/rtpdec_rfc4175.c index e9c62c1389..490db87520 100644 --- a/libavformat/rtpdec_rfc4175.c +++ b/libavformat/rtpdec_rfc4175.c @@ -205,8 +205,11 @@ static int rfc4175_handle_packet(AVFormatContext *ctx, PayloadContext *data, if (length > payload_len) length = payload_len; + if (line < 1) + return AVERROR_INVALIDDATA; + /* prevent ill-formed packets to write after buffer's end */ - copy_offset = (line * data->width + offset) * data->pgroup / data->xinc; + copy_offset = ((line - 1) * data->width + offset) * data->pgroup / data->xinc; if (copy_offset + length > data->frame_size) return AVERROR_INVALIDDATA;
The previous calculation code did not account for the fact that the copy_offset for the start of the frame array is at index 0, yet the scan line number from the rfc4175 RTP header starts at 1. This caused 2 issues to appear: - The first scan line was being copied into the array where the second scan line should be. This caused the resulting video to have a green line at the top of it. - Since the packet containing the last scan line would fail the calculation, the packet with the RTP marker would not be processed which caused a log message saying "Missed previous RTP marker" to be outputted for each frame. Signed-off-by: Jacob Siddall <kobe@live.com.au> --- Changes in v2: - Don't handle packet if the line number is less than 1 Section 12 in the VSF technical recommendation TR-03 specifies that the video scan line numbers should start at 1. http://www.videoservicesforum.org/download/technical_recommendations/VSF_TR-03_2015-11-12.pdf Changes in v3: - Changed the commit hash abbreviation in the patch file diff to be 10 characters in length rather than 7. This was causing the patch file to fail when it was applied. libavformat/rtpdec_rfc4175.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) -- 2.20.1