diff mbox

[FFmpeg-devel] Add support for Display Definition Segment to DVB Subtitle encoder

Message ID 20190711190653.56289-1-mikrohard@gmail.com
State Superseded
Headers show

Commit Message

mikrohard@gmail.com July 11, 2019, 7:06 p.m. UTC
Current version of dvbsub encoder doesn't support HD DVB subtitles. The high resolution bitmaps are muxed into the stream but without the DDS (display definition segment) the players asume that the DVB subtitles are in SD (720x576) resolution which causes them to either render the subtitles too large and misplaced or don't render them at all. By including the DDS as defined in section 7.7.1 of ETSI EN 300 743 (V1.3.1) this problem is fixed.

7.2.1 Display definition segment
The display definition for a subtitle service may be defined by the display definition segment if present in the stream.
Absence of a DDS implies that the stream is coded in accordance with EN 300 743 (V1.2.1) [5] and that a display width of 720 pixels and a display height of 576 lines may be assumed.

https://www.etsi.org/deliver/etsi_en/300700_300799/300743/01.03.01_60/en_300743v010301p.pdf

Signed-off-by: Jernej Fijacko <mikrohard@gmail.com>
---
 libavcodec/dvbsub.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Marton Balint July 11, 2019, 8:01 p.m. UTC | #1
On Thu, 11 Jul 2019, mikrohard@gmail.com wrote:

> Current version of dvbsub encoder doesn't support HD DVB subtitles. The high resolution bitmaps are muxed into the stream but without the DDS (display definition segment) the players asume that the DVB subtitles are in SD (720x576) resolution which causes them to either render the subtitles too large and misplaced or don't render them at all. By including the DDS as defined in section 7.7.1 of ETSI EN 300 743 (V1.3.1) this problem is fixed.
>
> 7.2.1 Display definition segment
> The display definition for a subtitle service may be defined by the display definition segment if present in the stream.
> Absence of a DDS implies that the stream is coded in accordance with EN 300 743 (V1.2.1) [5] and that a display width of 720 pixels and a display height of 576 lines may be assumed.
>
> https://www.etsi.org/deliver/etsi_en/300700_300799/300743/01.03.01_60/en_300743v010301p.pdf
>
> Signed-off-by: Jernej Fijacko <mikrohard@gmail.com>
> ---
> libavcodec/dvbsub.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/dvbsub.c b/libavcodec/dvbsub.c
> index 8cce702..c838567 100644
> --- a/libavcodec/dvbsub.c
> +++ b/libavcodec/dvbsub.c
> @@ -247,7 +247,8 @@ static void dvb_encode_rle8(uint8_t **pq,
>     *pq = q;
> }
> 
> -static int encode_dvb_subtitles(DVBSubtitleContext *s,
> +static int encode_dvb_subtitles(AVCodecContext *avctx,
> +                                DVBSubtitleContext *s,
>                                 uint8_t *outbuf, const AVSubtitle *h)
> {
>     uint8_t *q, *pseg_len;
> @@ -261,6 +262,19 @@ static int encode_dvb_subtitles(DVBSubtitleContext *s,
>     if (h->num_rects && !h->rects)
>         return -1;
> 
> +    if (avctx->width > 0 && avctx->height > 0) {
> +        /* display definition segment */
> +        *q++ = 0x0f; /* sync_byte */
> +        *q++ = 0x14; /* segment_type */
> +        bytestream_put_be16(&q, page_id);
> +        pseg_len = q;
> +        q += 2; /* segment length */
> +        *q++ = 0x20; /* dds version number & display window flag */

I guess 2 is the version number, right? Why 2?

> +        bytestream_put_be16(&q, avctx->width - 1); /* display width */
> +        bytestream_put_be16(&q, avctx->height - 1); /* display height */
> +        bytestream_put_be16(&pseg_len, q - pseg_len - 2);
> +    }
> +
>     /* page composition segment */
>
>     *q++ = 0x0f; /* sync_byte */
> @@ -449,7 +463,7 @@ static int dvbsub_encode(AVCodecContext *avctx,
>     DVBSubtitleContext *s = avctx->priv_data;
>     int ret;
> 
> -    ret = encode_dvb_subtitles(s, buf, sub);
> +    ret = encode_dvb_subtitles(avctx, s, buf, sub);

You could pass avctx only and assign s the same way as it is assigned 
here...

>     return ret;

Otherwise looks good.

Thanks,
Marton
Jernej July 11, 2019, 9:01 p.m. UTC | #2
To answer your questions...

1.) Yes, 2 is the version. To be honest I didn't really know what it meant.
I had a working sample stream & used the same values. But after reading the
spec I can see it's only used to signal changes in the display definition
segment which means it could be anything between 0 and 15 as it's going to
stay constant anyways. I can set it to 0 or 1 if you that's what you prefer.

2.) I saw that I could just pass avctx & read the priv_data inside
encode_dvb_subtitles function. I was just trying to make the least amount
of changes. But I'll change this and push a new patch tomorrow.

Regards,
Jernej

On Thu, Jul 11, 2019 at 10:01 PM Marton Balint <cus@passwd.hu> wrote:

>
>
> On Thu, 11 Jul 2019, mikrohard@gmail.com wrote:
>
> > Current version of dvbsub encoder doesn't support HD DVB subtitles. The
> high resolution bitmaps are muxed into the stream but without the DDS
> (display definition segment) the players asume that the DVB subtitles are
> in SD (720x576) resolution which causes them to either render the subtitles
> too large and misplaced or don't render them at all. By including the DDS
> as defined in section 7.7.1 of ETSI EN 300 743 (V1.3.1) this problem is
> fixed.
> >
> > 7.2.1 Display definition segment
> > The display definition for a subtitle service may be defined by the
> display definition segment if present in the stream.
> > Absence of a DDS implies that the stream is coded in accordance with EN
> 300 743 (V1.2.1) [5] and that a display width of 720 pixels and a display
> height of 576 lines may be assumed.
> >
> >
> https://www.etsi.org/deliver/etsi_en/300700_300799/300743/01.03.01_60/en_300743v010301p.pdf
> >
> > Signed-off-by: Jernej Fijacko <mikrohard@gmail.com>
> > ---
> > libavcodec/dvbsub.c | 18 ++++++++++++++++--
> > 1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/dvbsub.c b/libavcodec/dvbsub.c
> > index 8cce702..c838567 100644
> > --- a/libavcodec/dvbsub.c
> > +++ b/libavcodec/dvbsub.c
> > @@ -247,7 +247,8 @@ static void dvb_encode_rle8(uint8_t **pq,
> >     *pq = q;
> > }
> >
> > -static int encode_dvb_subtitles(DVBSubtitleContext *s,
> > +static int encode_dvb_subtitles(AVCodecContext *avctx,
> > +                                DVBSubtitleContext *s,
> >                                 uint8_t *outbuf, const AVSubtitle *h)
> > {
> >     uint8_t *q, *pseg_len;
> > @@ -261,6 +262,19 @@ static int encode_dvb_subtitles(DVBSubtitleContext
> *s,
> >     if (h->num_rects && !h->rects)
> >         return -1;
> >
> > +    if (avctx->width > 0 && avctx->height > 0) {
> > +        /* display definition segment */
> > +        *q++ = 0x0f; /* sync_byte */
> > +        *q++ = 0x14; /* segment_type */
> > +        bytestream_put_be16(&q, page_id);
> > +        pseg_len = q;
> > +        q += 2; /* segment length */
> > +        *q++ = 0x20; /* dds version number & display window flag */
>
> I guess 2 is the version number, right? Why 2?
>
> > +        bytestream_put_be16(&q, avctx->width - 1); /* display width */
> > +        bytestream_put_be16(&q, avctx->height - 1); /* display height */
> > +        bytestream_put_be16(&pseg_len, q - pseg_len - 2);
> > +    }
> > +
> >     /* page composition segment */
> >
> >     *q++ = 0x0f; /* sync_byte */
> > @@ -449,7 +463,7 @@ static int dvbsub_encode(AVCodecContext *avctx,
> >     DVBSubtitleContext *s = avctx->priv_data;
> >     int ret;
> >
> > -    ret = encode_dvb_subtitles(s, buf, sub);
> > +    ret = encode_dvb_subtitles(avctx, s, buf, sub);
>
> You could pass avctx only and assign s the same way as it is assigned
> here...
>
> >     return ret;
>
> Otherwise looks good.
>
> Thanks,
> Marton
> _______________________________________________
> 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 mbox

Patch

diff --git a/libavcodec/dvbsub.c b/libavcodec/dvbsub.c
index 8cce702..c838567 100644
--- a/libavcodec/dvbsub.c
+++ b/libavcodec/dvbsub.c
@@ -247,7 +247,8 @@  static void dvb_encode_rle8(uint8_t **pq,
     *pq = q;
 }
 
-static int encode_dvb_subtitles(DVBSubtitleContext *s,
+static int encode_dvb_subtitles(AVCodecContext *avctx,
+                                DVBSubtitleContext *s,
                                 uint8_t *outbuf, const AVSubtitle *h)
 {
     uint8_t *q, *pseg_len;
@@ -261,6 +262,19 @@  static int encode_dvb_subtitles(DVBSubtitleContext *s,
     if (h->num_rects && !h->rects)
         return -1;
 
+    if (avctx->width > 0 && avctx->height > 0) {
+        /* display definition segment */
+        *q++ = 0x0f; /* sync_byte */
+        *q++ = 0x14; /* segment_type */
+        bytestream_put_be16(&q, page_id);
+        pseg_len = q;
+        q += 2; /* segment length */
+        *q++ = 0x20; /* dds version number & display window flag */
+        bytestream_put_be16(&q, avctx->width - 1); /* display width */
+        bytestream_put_be16(&q, avctx->height - 1); /* display height */
+        bytestream_put_be16(&pseg_len, q - pseg_len - 2);
+    }
+
     /* page composition segment */
 
     *q++ = 0x0f; /* sync_byte */
@@ -449,7 +463,7 @@  static int dvbsub_encode(AVCodecContext *avctx,
     DVBSubtitleContext *s = avctx->priv_data;
     int ret;
 
-    ret = encode_dvb_subtitles(s, buf, sub);
+    ret = encode_dvb_subtitles(avctx, s, buf, sub);
     return ret;
 }