[FFmpeg-devel] [PATCH 2/2 v2] lavf/mpegts: add reading of ARIB data coding descriptor

[Jan Ekström]

On Sun, Feb 3, 2019 at 3:33 PM Jan Ekström <jeebjp at gmail.com> wrote:
>
> On Sun, Feb 3, 2019 at 3:12 PM Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
> >
> > 2019-02-03 1:39 GMT+01:00, Jan Ekström <jeebjp at gmail.com>:
> > > This enables us to read the data coding type utilized for
> > > a specific private data stream, of which we currently are
> > > interested in ARIB caption streams.
> > >
> > > The component tag limitations are according to ARIB TR-B14,
> > > and the component IDs are defined in ARIB STD-B10.
> > > ---
> > >  libavformat/mpegts.c  | 44 +++++++++++++++++++++++++++++++++++++++++++
> > >  libavformat/version.h |  2 +-
> > >  2 files changed, 45 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> > > index 300db110d4..f9dc04eb52 100644
> > > --- a/libavformat/mpegts.c
> > > +++ b/libavformat/mpegts.c
> > > @@ -2013,6 +2013,50 @@ int ff_parse_mpeg2_descriptor(AVFormatContext *fc,
> > > AVStream *st, int stream_type
> > >              }
> > >          }
> > >          break;
> > > +    case 0xfd: /* ARIB data coding type descriptor */
> > > +        // STD-B24, fascicle 3, chapter 4 defines private_stream_1
> > > +        // for captions
> > > +        if (stream_type == STREAM_TYPE_PRIVATE_DATA) {
> > > +            // This structure is defined in STD-B10, part 1, listing 5.4
> > > and
> > > +            // part 2, 6.2.20).
> > > +            // Listing of data_component_ids is in STD-B10, part 2, Annex
> > > J.
> > > +            // Component tag limits are documented in TR-B14, fascicle 2,
> > > +            // Vol. 3, Section 2, 4.2.8.1
> > > +            int actual_component_tag = st->stream_identifier - 1;
> > > +            int picked_profile = FF_PROFILE_UNKNOWN;
> > > +            int data_component_id = get16(pp, desc_end);
> >
> > > +            if (data_component_id < 0)
> > > +                return AVERROR_INVALIDDATA;
> >
> > What is the effect of this return if it happens?
> > Why isn't this just a "break" like below?
> >
>
> I think this might have been a half-automatic thing on my behalf after
> seeing that get16 does indeed return an error value if it can't read
> that amount of data.
>
> Although, it seems like there is no concise way of handling errors
> even in this function (`ff_parse_mpeg2_descriptor`):
> 1. 0x1E breaks.
> 2. 0x56, 0x59 set AVERROR_INVALIDDATA if the descriptor is not long
> enough (pointer check instead of getXX though).
> 3. 0x26 does an exact equality check on a specific value and ignores
> result otherwise.
> 4. 0x7f does a getXX and then returns AVERROR_INVALIDDATA if it fails
> (this is getting similar to what I did).
>
> Personally I don't have heavy feelings about how I exit this function,
> but I feel like if there's not enough data and we've gotten as far as
> we've got, returning "invalid data" is not incorrect (and has a
> precedent).
>

Also sorry, I just noticed the two different types of exit. I guess I
did just a break there since we seem to still have a valid structure,
but we're just not interested in that specific coding type.

(a list of them can be seen in the specs I noted).

Jan