| 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() | expand |
| 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 |
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".
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". >
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".
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.
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".
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.
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
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".
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.
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 --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;