[FFmpeg-devel] [PATCH 7/8] avformat/mpegts: Fix for the DOVI video stream descriptor
lance.lmwang at gmail.com
lance.lmwang at gmail.com
Thu Oct 28 04:28:08 EEST 2021
On Wed, Oct 27, 2021 at 10:54:31AM -0400, quietvoid wrote:
> On Wed, Oct 27, 2021 at 10:26 AM <lance.lmwang at 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 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).
> >
> > 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 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".
--
Thanks,
Limin Wang
More information about the ffmpeg-devel
mailing list