Re: [FFmpeg-devel] [PATCH 4/6] lavc: Add coded bitstream read/write support for VP9
On 5/1/2018 9:53 PM, Mark Thompson wrote: > On 01/05/18 18:33, James Almer wrote: >> On 4/30/2018 8:26 PM, Mark Thompson wrote: >>> --- >>> Main change since last time is including the array subscripts. Constants >>> are also cleaned up a bit, but stay in the cbs header (vp9.h could probably >>> be taken over for this purpose, but currently it's an unnamespaced header >>> used by the decoder so I haven't touched it). >>> >>> >>> configure| 2 + >>> doc/bitstream_filters.texi | 2 +- >>> libavcodec/Makefile | 1 + >>> libavcodec/cbs.c | 6 + >>> libavcodec/cbs.h | 1 + >>> libavcodec/cbs_internal.h| 1 + >>> libavcodec/cbs_vp9.c | 679 >>> +++ >>> libavcodec/cbs_vp9.h | 201 +++ >>> libavcodec/cbs_vp9_syntax_template.c | 390 >>> 9 files changed, 1282 insertions(+), 1 deletion(-) >>> create mode 100644 libavcodec/cbs_vp9.c >>> create mode 100644 libavcodec/cbs_vp9.h >>> create mode 100644 libavcodec/cbs_vp9_syntax_template.c >> >> LGTM. No apparent data copy on any of the read methods which is very >> promising for the AV1 implementation. >> Only CodedBitstreamType->write_unit() still seems a tad sub-optimal with >> the temp write_buffer, especially in this module where unlike >> mpeg2/h2645 you memcpy the data twice. > > It's copied twice in MPEG-2/H.26[45] as well - once from the write buffer to > the unit data buffers, then a second time to combine them into the fragment > data buffer (which also adds the emulation prevention in the H.26[45] case, > so not really a direct copy). It's probably impossible to avoid the memcpys when assembling a fragment out of each unit data for most codecs, so we should focus on optimizing write_unit(). > > It's not very easy to do better - a possible way would be to allow the write > buffer to contain multiple units and make the reference point at the write > buffer itself, then we could avoid the first copy to a per-unit buffer. That > requires somewhat more care in writing, though, because some special cases > become nastier to handle (re-copying previous units on overflow; allocating a > new write buffer if unit data needs to live beyond the reassembly). > > In theory that can also elide the second copy in VP9 (because the frames are > just concatenated in the write buffer, and the superframe header placed at > the end), but in practice that isn't necessarily a benefit because you end up > with even more allocation if the following component ever needs to hold more > than one output packet at once. One option in write_unit() could be making the unit take ownership of the buffer as filled by the codec specific bitstream functions (what's currently priv->write_buffer) instead of allocating a new one within ff_cbs_alloc_unit_data and then copying data to it. This would make priv->write_buffer something local to write_unit(), allocated anew once per call, and then wrapped as the unit's buffer. This could be done with a new ff_cbs_wrap_unit_data() function or similar. > > So, I'm not intending to do anything with this for now, but if you want to > look at it (or have any other ideas you think I should pursue) then please do. > > Thanks, > > - Mark > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/6] lavc: Add coded bitstream read/write support for VP9
On 01/05/18 18:33, James Almer wrote: > On 4/30/2018 8:26 PM, Mark Thompson wrote: >> --- >> Main change since last time is including the array subscripts. Constants >> are also cleaned up a bit, but stay in the cbs header (vp9.h could probably >> be taken over for this purpose, but currently it's an unnamespaced header >> used by the decoder so I haven't touched it). >> >> >> configure| 2 + >> doc/bitstream_filters.texi | 2 +- >> libavcodec/Makefile | 1 + >> libavcodec/cbs.c | 6 + >> libavcodec/cbs.h | 1 + >> libavcodec/cbs_internal.h| 1 + >> libavcodec/cbs_vp9.c | 679 >> +++ >> libavcodec/cbs_vp9.h | 201 +++ >> libavcodec/cbs_vp9_syntax_template.c | 390 >> 9 files changed, 1282 insertions(+), 1 deletion(-) >> create mode 100644 libavcodec/cbs_vp9.c >> create mode 100644 libavcodec/cbs_vp9.h >> create mode 100644 libavcodec/cbs_vp9_syntax_template.c > > LGTM. No apparent data copy on any of the read methods which is very > promising for the AV1 implementation. > Only CodedBitstreamType->write_unit() still seems a tad sub-optimal with > the temp write_buffer, especially in this module where unlike > mpeg2/h2645 you memcpy the data twice. It's copied twice in MPEG-2/H.26[45] as well - once from the write buffer to the unit data buffers, then a second time to combine them into the fragment data buffer (which also adds the emulation prevention in the H.26[45] case, so not really a direct copy). It's not very easy to do better - a possible way would be to allow the write buffer to contain multiple units and make the reference point at the write buffer itself, then we could avoid the first copy to a per-unit buffer. That requires somewhat more care in writing, though, because some special cases become nastier to handle (re-copying previous units on overflow; allocating a new write buffer if unit data needs to live beyond the reassembly). In theory that can also elide the second copy in VP9 (because the frames are just concatenated in the write buffer, and the superframe header placed at the end), but in practice that isn't necessarily a benefit because you end up with even more allocation if the following component ever needs to hold more than one output packet at once. So, I'm not intending to do anything with this for now, but if you want to look at it (or have any other ideas you think I should pursue) then please do. Thanks, - Mark ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/6] lavc: Add coded bitstream read/write support for VP9
On 4/30/2018 8:26 PM, Mark Thompson wrote: > --- > Main change since last time is including the array subscripts. Constants are > also cleaned up a bit, but stay in the cbs header (vp9.h could probably be > taken over for this purpose, but currently it's an unnamespaced header used > by the decoder so I haven't touched it). > > > configure| 2 + > doc/bitstream_filters.texi | 2 +- > libavcodec/Makefile | 1 + > libavcodec/cbs.c | 6 + > libavcodec/cbs.h | 1 + > libavcodec/cbs_internal.h| 1 + > libavcodec/cbs_vp9.c | 679 > +++ > libavcodec/cbs_vp9.h | 201 +++ > libavcodec/cbs_vp9_syntax_template.c | 390 > 9 files changed, 1282 insertions(+), 1 deletion(-) > create mode 100644 libavcodec/cbs_vp9.c > create mode 100644 libavcodec/cbs_vp9.h > create mode 100644 libavcodec/cbs_vp9_syntax_template.c LGTM. No apparent data copy on any of the read methods which is very promising for the AV1 implementation. Only CodedBitstreamType->write_unit() still seems a tad sub-optimal with the temp write_buffer, especially in this module where unlike mpeg2/h2645 you memcpy the data twice. Can't really comment on the actual bitstream parsing code, but trace_headers_bsf doesn't complain about any of the samples i tried. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
