[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: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).

Jan