diff mbox series

[FFmpeg-devel,7/8] avformat/mpegts: Fix for the DOVI video stream descriptor

Message ID 1634216942-20329-7-git-send-email-lance.lmwang@gmail.com
State Accepted
Commit 08c688e64dce81d202bf228291eb128eec83a392
Headers show
Series [FFmpeg-devel,1/8] avfilter/af_replaygain: use fabsf() instead of fabs()
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Limin Wang Oct. 14, 2021, 1:09 p.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

By <<Dolby Vision Streams Within the MPEG-2 Transport Stream Format v1.2>>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/mpegts.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Limin Wang Oct. 14, 2021, 1:36 p.m. UTC | #1
On Thu, Oct 14, 2021 at 09:18:16AM -0400, f tcChlisop0 wrote:
> On Thu, Oct 14, 2021 at 9:10 AM <lance.lmwang@gmail.com> wrote:
> 
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > By <<Dolby Vision Streams Within the MPEG-2 Transport Stream Format v1.2>>
> >
> > Signed-off-by: Limin Wang <lance.lmwang@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@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@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.


> 
> thanks, quietvoid
> _______________________________________________
> 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".
quietvoid Oct. 14, 2021, 1:45 p.m. UTC | #2
On Thu, Oct 14, 2021 at 9:36 AM <lance.lmwang@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@gmail.com> wrote:
> >
> > > From: Limin Wang <lance.lmwang@gmail.com>
> > >
> > > By <<Dolby Vision Streams Within the MPEG-2 Transport Stream Format
> v1.2>>
> > >
> > > Signed-off-by: Limin Wang <lance.lmwang@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@ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request@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.

>
> > thanks, quietvoid
> > _______________________________________________
> > 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".
>
> --
> Thanks,
> Limin Wang
> _______________________________________________
> 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".
>
Limin Wang Oct. 14, 2021, 1:52 p.m. UTC | #3
On Thu, Oct 14, 2021 at 09:45:45AM -0400, quietvoid wrote:
> On Thu, Oct 14, 2021 at 9:36 AM <lance.lmwang@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@gmail.com> wrote:
> > >
> > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > >
> > > > By <<Dolby Vision Streams Within the MPEG-2 Transport Stream Format
> > v1.2>>
> > > >
> > > > Signed-off-by: Limin Wang <lance.lmwang@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@ffmpeg.org
> > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > >
> > > > To unsubscribe, visit link above, or email
> > > > ffmpeg-devel-request@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.


> 
> >
> > > thanks, quietvoid
> > > _______________________________________________
> > > 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".
> >
> > --
> > Thanks,
> > Limin Wang
> > _______________________________________________
> > 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".
> >
> _______________________________________________
> 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".
quietvoid Oct. 14, 2021, 2:12 p.m. UTC | #4
On Thu, Oct 14, 2021 at 9:52 AM <lance.lmwang@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@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@gmail.com> wrote:
> > > >
> > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > >
> > > > > By <<Dolby Vision Streams Within the MPEG-2 Transport Stream Format
> > > v1.2>>
> > > > >
> > > > > Signed-off-by: Limin Wang <lance.lmwang@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@ffmpeg.org
> > > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > >
> > > > > To unsubscribe, visit link above, or email
> > > > > ffmpeg-devel-request@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 I still went ahead with it to avoid the duplicated code.
I've had no response since then, though.

Either way, I'll just wait for your patches to get in.
Thanks for the review.
Limin Wang Oct. 14, 2021, 2:30 p.m. UTC | #5
On Thu, Oct 14, 2021 at 10:12:05AM -0400, quietvoid wrote:
> On Thu, Oct 14, 2021 at 9:52 AM <lance.lmwang@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@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@gmail.com> wrote:
> > > > >
> > > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > >
> > > > > > By <<Dolby Vision Streams Within the MPEG-2 Transport Stream Format
> > > > v1.2>>
> > > > > >
> > > > > > Signed-off-by: Limin Wang <lance.lmwang@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@ffmpeg.org
> > > > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > >
> > > > > > To unsubscribe, visit link above, or email
> > > > > > ffmpeg-devel-request@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.

> 
> But I still went ahead with it to avoid the duplicated code.
> I've had no response since then, though.
> 
> Either way, I'll just wait for your patches to get in.
> Thanks for the review.
> _______________________________________________
> 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".
Limin Wang Oct. 27, 2021, 1:24 a.m. UTC | #6
On Thu, Oct 14, 2021 at 09:09:01PM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> By <<Dolby Vision Streams Within the MPEG-2 Transport Stream Format v1.2>>
> 
> Signed-off-by: Limin Wang <lance.lmwang@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
> 

will apply patch#5,#6,#7 tomorrow unless there are objections.
Jan Ekström Oct. 27, 2021, 8:50 a.m. UTC | #7
On Thu, Oct 14, 2021 at 5:31 PM <lance.lmwang@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@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@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@gmail.com> wrote:
> > > > > >
> > > > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > > >
> > > > > > > By <<Dolby Vision Streams Within the MPEG-2 Transport Stream Format
> > > > > v1.2>>
> > > > > > >
> > > > > > > Signed-off-by: Limin Wang <lance.lmwang@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@ffmpeg.org
> > > > > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > > >
> > > > > > > To unsubscribe, visit link above, or email
> > > > > > > ffmpeg-devel-request@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
Limin Wang Oct. 27, 2021, 2:25 p.m. UTC | #8
On Wed, Oct 27, 2021 at 11:50:04AM +0300, Jan Ekström wrote:
> On Thu, Oct 14, 2021 at 5:31 PM <lance.lmwang@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@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@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@gmail.com> wrote:
> > > > > > >
> > > > > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > > > >
> > > > > > > > By <<Dolby Vision Streams Within the MPEG-2 Transport Stream Format
> > > > > > v1.2>>
> > > > > > > >
> > > > > > > > Signed-off-by: Limin Wang <lance.lmwang@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@ffmpeg.org
> > > > > > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > > > >
> > > > > > > > To unsubscribe, visit link above, or email
> > > > > > > > ffmpeg-devel-request@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).

I haven't got such sample which use the dependency_pid yet, most of sample set bl_present_flag
as 1. MP4 use DOVIDecoderConfigurationRecord, but mpegts use DOVI_video_stream_descriptor. if
we want to keep all information, the side data of DOVI is better to use DOVI_video_stream_descriptor.
If we add one more field to DOVIDecoderConfigurationRecord, I think it's misleading. 

> 
> Jan
> _______________________________________________
> 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".
quietvoid Oct. 27, 2021, 2:54 p.m. UTC | #9
On Wed, Oct 27, 2021 at 10:26 AM <lance.lmwang@gmail.com> wrote:

> On Wed, Oct 27, 2021 at 11:50:04AM +0300, Jan Ekström wrote:
> > On Thu, Oct 14, 2021 at 5:31 PM <lance.lmwang@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@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@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@gmail.com>
> wrote:
> > > > > > > >
> > > > > > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > > > > >
> > > > > > > > > By <<Dolby Vision Streams Within the MPEG-2 Transport
> Stream Format
> > > > > > > v1.2>>
> > > > > > > > >
> > > > > > > > > Signed-off-by: Limin Wang <lance.lmwang@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@ffmpeg.org
> > > > > > > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > > > > >
> > > > > > > > > To unsubscribe, visit link above, or email
> > > > > > > > > ffmpeg-devel-request@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).
>
> I haven't got such sample which use the dependency_pid yet, most of sample
> set bl_present_flag
> as 1. MP4 use DOVIDecoderConfigurationRecord, but mpegts use
> DOVI_video_stream_descriptor. if
> we want to keep all information, the side data of DOVI is better to use
> DOVI_video_stream_descriptor.
> If we add one more field to DOVIDecoderConfigurationRecord, I think it's
> misleading.
>
>
I tested the patch on my side with this file: https://0x0.st/-nzv.ts
It was created from the test samples by MakeMKV, trimmed, demuxed into two
streams and remuxed with tsMuxer.

The output from ffprobe for the 2nd track looks correct to me:
DOVI, version: 1.0, profile: 7, level: 6, rpu flag: 1, el flag: 1, bl flag:
0, dependency_pid: 4113, compatibility id: 6

Thank you.
Limin Wang Oct. 28, 2021, 1:28 a.m. UTC | #10
On Wed, Oct 27, 2021 at 10:54:31AM -0400, quietvoid wrote:
> On Wed, Oct 27, 2021 at 10:26 AM <lance.lmwang@gmail.com> wrote:
> 
> > On Wed, Oct 27, 2021 at 11:50:04AM +0300, Jan Ekström wrote:
> > > On Thu, Oct 14, 2021 at 5:31 PM <lance.lmwang@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@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@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@gmail.com>
> > wrote:
> > > > > > > > >
> > > > > > > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > > > > > >
> > > > > > > > > > By <<Dolby Vision Streams Within the MPEG-2 Transport
> > Stream Format
> > > > > > > > v1.2>>
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Limin Wang <lance.lmwang@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@ffmpeg.org
> > > > > > > > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > > > > > > >
> > > > > > > > > > To unsubscribe, visit link above, or email
> > > > > > > > > > ffmpeg-devel-request@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).
> >
> > I haven't got such sample which use the dependency_pid yet, most of sample
> > set bl_present_flag
> > as 1. MP4 use DOVIDecoderConfigurationRecord, but mpegts use
> > DOVI_video_stream_descriptor. if
> > we want to keep all information, the side data of DOVI is better to use
> > DOVI_video_stream_descriptor.
> > If we add one more field to DOVIDecoderConfigurationRecord, I think it's
> > misleading.
> >
> >
> I tested the patch on my side with this file: https://0x0.st/-nzv.ts
> It was created from the test samples by MakeMKV, trimmed, demuxed into two
> streams and remuxed with tsMuxer.
> 
> The output from ffprobe for the 2nd track looks correct to me:
> DOVI, version: 1.0, profile: 7, level: 6, rpu flag: 1, el flag: 1, bl flag:
> 0, dependency_pid: 4113, compatibility id: 6

Thanks for the testing, have applied the patch.

> 
> Thank you.
> _______________________________________________
> 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".
diff mbox series

Patch

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;