[FFmpeg-devel] [PATCH] [HACK] mxfdec: support EIA-608 (closed caption subtitles).

[Reimar Döffinger]

On Thu, Apr 19, 2012 at 02:05:31PM +0200, Tomas Härdin wrote:
> On Thu, 2012-04-12 at 21:45 +0200, Reimar Döffinger wrote:
> > This is a hack because:
> > 1) The way to detect this subtitle type is very questionable,
> >    and might cause other subtitle types to be misdetected
> >    as EIA-608
> 
> You should check EssenceContainer or whatever is the equivalent to
> PictureEssenceCoding for subtitles.

I didn't find anything relevant, though I might just miss the
right identifier to look for.

> > 2) The subtitle packets do not get any pts/dts values assigned
> 
> I suspect mxf->current_edit_unit should suffice.

I had a hack to just make it use the video code, but I didn't see
value in cluttering this initial patch with more guesses that
are not necessary for a working proof-of-concept.

> > 3) I don't have the slightest clue what those extra 32 bytes
> >    I throw away are good for
> 
> Please get a clue about that. In fact, try to find the relevant specs
> for this (both the subtitle spec and the corresponding SMPTE mapping
> document).

I couldn't find anything useful, that's the main reason for sending this
to the list.

> > --- a/libavformat/avienc.c
> > +++ b/libavformat/avienc.c
> > @@ -234,7 +234,7 @@ static int avi_write_header(AVFormatContext *s)
> >          case AVMEDIA_TYPE_SUBTITLE:
> >              // XSUB subtitles behave like video tracks, other subtitles
> >              // are not (yet) supported.
> > -            if (stream->codec_id != CODEC_ID_XSUB) {
> > +            if (0 && stream->codec_id != CODEC_ID_XSUB) {
> >                  av_log(s, AV_LOG_ERROR, "Subtitle streams other than DivX XSUB are not supported by the AVI muxer.\n");
> >                  return AVERROR_PATCHWELCOME;
> >              }
> 
> Unrelated?

Was just some debugging help.

> > @@ -2043,6 +2045,12 @@ static int mxf_read_packet_old(AVFormatContext *s, AVPacket *pkt)
> >                      av_log(s, AV_LOG_ERROR, "error reading D-10 aes3 frame\n");
> >                      return AVERROR_INVALIDDATA;
> >                  }
> > +            } else if (st->codec->codec_id == CODEC_ID_EIA_608 && klv.length > 28 + 4) {
> > +                avio_skip(s->pb, 28);
> > +                ret = av_get_packet(s->pb, pkt, klv.length - 28 - 4);
> > +                if (ret < 0)
> > +                    return ret;
> > +                avio_skip(s->pb, 4);
> >              } else {
> >                  ret = av_get_packet(s->pb, pkt, klv.length);
> >                  if (ret < 0)
> > @@ -2117,7 +2125,14 @@ static int mxf_read_packet(AVFormatContext *s, AVPacket *pkt)
> >      if ((ret64 = avio_seek(s->pb, pos, SEEK_SET)) < 0)
> >          return ret64;
> >  
> > -    if ((ret = av_get_packet(s->pb, pkt, size)) < 0)
> > +    if (st->codec->codec_id == CODEC_ID_EIA_608 && size > 28 + 4) {
> > +        avio_skip(s->pb, 28);
> > +        ret = av_get_packet(s->pb, pkt, size - 28 - 4);
> > +        avio_skip(s->pb, 4);
> > +    } else
> > +        ret = av_get_packet(s->pb, pkt, size);
> > +
> > +    if (ret < 0)
> 
> Fold these two hunks into a common function.

I'm not sure that will give a nice result.
More importantly though there's no point in it until it's clear what
should be in there (just skipping seems unlikely to really be the right
approach).