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

quietvoid tcchlisop0 at gmail.com
Thu Oct 14 17:12:05 EEST 2021


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 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.


More information about the ffmpeg-devel mailing list