[FFmpeg-devel] [PATCH 7/8] avformat/mpegts: Fix for the DOVI video stream descriptor
Jan Ekström
jeebjp at gmail.com
Wed Oct 27 11:50:04 EEST 2021
On Thu, Oct 14, 2021 at 5:31 PM <lance.lmwang at gmail.com> wrote:
>
> On Thu, Oct 14, 2021 at 10:12:05AM -0400, quietvoid wrote:
> > On Thu, Oct 14, 2021 at 9:52 AM <lance.lmwang at gmail.com> wrote:
> >
> > > On Thu, Oct 14, 2021 at 09:45:45AM -0400, quietvoid wrote:
> > > > On Thu, Oct 14, 2021 at 9:36 AM <lance.lmwang at gmail.com> wrote:
> > > >
> > > > > On Thu, Oct 14, 2021 at 09:18:16AM -0400, f tcChlisop0 wrote:
> > > > > > On Thu, Oct 14, 2021 at 9:10 AM <lance.lmwang at gmail.com> wrote:
> > > > > >
> > > > > > > From: Limin Wang <lance.lmwang at gmail.com>
> > > > > > >
> > > > > > > By <<Dolby Vision Streams Within the MPEG-2 Transport Stream Format
> > > > > v1.2>>
> > > > > > >
> > > > > > > Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
> > > > > > > ---
> > > > > > > libavformat/mpegts.c | 11 +++++++++--
> > > > > > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> > > > > > > index 44d9298..774964d 100644
> > > > > > > --- a/libavformat/mpegts.c
> > > > > > > +++ b/libavformat/mpegts.c
> > > > > > > @@ -2178,6 +2178,8 @@ int ff_parse_mpeg2_descriptor(AVFormatContext
> > > > > *fc,
> > > > > > > AVStream *st, int stream_type
> > > > > > > AVDOVIDecoderConfigurationRecord *dovi;
> > > > > > > size_t dovi_size;
> > > > > > > int ret;
> > > > > > > + int dependency_pid;
> > > > > > > +
> > > > > > > if (desc_end - *pp < 4) // (8 + 8 + 7 + 6 + 1 + 1 +
> > > 1) / 8
> > > > > > > return AVERROR_INVALIDDATA;
> > > > > > >
> > > > > > > @@ -2193,7 +2195,11 @@ int
> > > ff_parse_mpeg2_descriptor(AVFormatContext
> > > > > *fc,
> > > > > > > AVStream *st, int stream_type
> > > > > > > dovi->rpu_present_flag = (buf >> 2) & 0x01; // 1
> > > bit
> > > > > > > dovi->el_present_flag = (buf >> 1) & 0x01; // 1
> > > bit
> > > > > > > dovi->bl_present_flag = buf & 0x01; // 1
> > > bit
> > > > > > > - if (desc_end - *pp >= 20) { // 4 + 4 * 4
> > > > > > > + if (!dovi->bl_present_flag && desc_end - *pp >= 2) {
> > > > > > > + buf = get16(pp, desc_end);
> > > > > > > + dependency_pid = buf >> 3; // 13 bits
> > > > > > > + }
> > > > > > > + if (desc_end - *pp >= 1) { // 8 bits
> > > > > > > buf = get8(pp, desc_end);
> > > > > > > dovi->dv_bl_signal_compatibility_id = (buf >> 4) &
> > > > > 0x0f;
> > > > > > > // 4 bits
> > > > > > > } else {
> > > > > > > @@ -2210,12 +2216,13 @@ int
> > > ff_parse_mpeg2_descriptor(AVFormatContext
> > > > > *fc,
> > > > > > > AVStream *st, int stream_type
> > > > > > > }
> > > > > > >
> > > > > > > av_log(fc, AV_LOG_TRACE, "DOVI, version: %d.%d,
> > > profile:
> > > > > %d,
> > > > > > > level: %d, "
> > > > > > > - "rpu flag: %d, el flag: %d, bl flag: %d,
> > > > > compatibility
> > > > > > > id: %d\n",
> > > > > > > + "rpu flag: %d, el flag: %d, bl flag: %d,
> > > > > > > dependency_pid: %d, compatibility id: %d\n",
> > > > > > > dovi->dv_version_major, dovi->dv_version_minor,
> > > > > > > dovi->dv_profile, dovi->dv_level,
> > > > > > > dovi->rpu_present_flag,
> > > > > > > dovi->el_present_flag,
> > > > > > > dovi->bl_present_flag,
> > > > > > > + dependency_pid,
> > > > > > > dovi->dv_bl_signal_compatibility_id);
> > > > > > > }
> > > > > > > break;
> > > > > > > --
> > > > > > > 1.8.3.1
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > 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".
> > > > > > >
> > > > > >
> > > > > > Hi, this is something I had fixed in this patchset:
> > > > > > https://ffmpeg.org/pipermail/ffmpeg-devel/2021-September/286234.html
> > > > > > However the dependency_pid is ignored, as it has no use presently.
> > > > > >
> > > > > > Which patch should take precedence?
> > > > >
> > > > > Sorry, I have noticed your patch before. By the quick review of your
> > > patch,
> > > > > it's a lot function change and difficult to merge I think. I prefer to
> > > > > fix the issue with existing code first instead of mixed function
> > > change.
> > > > >
> > > >
> > > > Okay, that makes sense.
> > > > I will wait and rebase before resending for review then.
> > > >
> > > > However I'm worried my patch will still result in ignoring
> > > dependency_pid,
> > > > because it is not part of AVDOVIDecoderConfigurationRecord, unless it is
> > > > added.
> > >
> > > I failed to find your patchset in my email archive, so I reply it here for
> > > my comments:
> > > - if (ret < 0) {
> > > - av_free(dovi);
> > > + if ((ret = ff_isom_parse_dvcc_dvvc(fc, st, *pp, desc_len, 1))
> > > < 0)
> > > return ret;
> > > - }
> > >
> > > I think it's wrong to use ff_isom_parse_dvcc_dvvc() here for mpegts
> > > use DOVIVideoStreamDescriptor, ISOM use DOVIDecoderConfigurationRecord.
> > > they're different syntax if you have checked the two specs. So your
> > > parsing isn't
> > > follow the specs as dependency_pid is used by DOVIVideoStreamDescriptor.
> > >
> >
> > That's true, however they only differ by dependency_pid.
> > This is a concern I've noted here:
> > https://ffmpeg.org/pipermail/ffmpeg-devel/2021-September/286159.html
>
> But they're different descriptor anyway, in fact only the first 4 bytes is same,
> after that:
> DOVI_video_stream_descriptor:
> if (!bl_present_flag) {
> dependency_pid 13 bits
> reserved 3 bits
> }
> dv_bl_signal_compatibility_id 4 bits
> reserved 4 bits
>
> DOVIDecoderConfigurationRecord
> dv_bl_signal_compatibility_id 4 bits
> reserved 28 bits
> reserved 32*4 bits
>
> Now the side data prefer to use DOVIDecoderConfigurationRecord, so mpegts should
> parse by the DOVI_video_stream_descriptor syntax and store the result in
> DOVIDecoderConfigurationRecord only.
>
If the MPEG-TS stuff informs one regarding the other PID to utilize
for EL use cases, we might want to extend the configuration record to
either contain an AVStream * pointer, or a stream index (or stream
id), so it can be referred to by the API client? And then when writing
the metadata to MP4 we would have to not allow it, since I think the
MP4 side doesn't allow it (unless I recall the DoVi MP4 spec
incorrectly).
Jan
More information about the ffmpeg-devel
mailing list