diff mbox series

[FFmpeg-devel,v2] dvbsub.c, change segment order to be in line with spec

Message ID 1901993192.202425.1582021627225.JavaMail.zimbra@fora.si
State New
Headers show
Series [FFmpeg-devel,v2] dvbsub.c, change segment order to be in line with spec | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Alen Vrečko Feb. 18, 2020, 10:27 a.m. UTC 
On https://patchwork.ffmpeg.org/project/ffmpeg/list/ the new patch doesn't show up. I used the same filename as before.

Just in case sending it again with changed filename (and also changed commit message).

Sorry for the extra message.

Alen

----- Original Message -----
From: "Alen Vrečko" <alen.vrecko@fora.si>
To: "ffmpeg-devel" <ffmpeg-devel@ffmpeg.org>
Sent: Tuesday, February 18, 2020 11:15:04 AM
Subject: Re: [FFmpeg-devel] dvbsub.c, change segment order to be in line with spec

Sure thing. I attached a new patch.

Thank you.

Alen

----- Original Message -----
From: "Carl Eugen Hoyos" <ceffmpeg@gmail.com>
To: "ffmpeg-devel" <ffmpeg-devel@ffmpeg.org>
Sent: Monday, February 17, 2020 6:28:17 PM
Subject: Re: [FFmpeg-devel] dvbsub.c,	change segment order to be in line with spec

> Am 17.02.2020 um 16:10 schrieb Alen Vrečko <alen.vrecko@fora.si>:
> 
> Hello, everyone.
> 
> I'd like to request a change in order of DVB segments to be in line with ETSI 300 743. Basically Region Composition Segment should come before CLUT segment. This caused problems on legacy STBs namely Amino A110. 
> 
> In the ETSI EN 300 743 V1.6.1 (2018-10) section 4.4 it is written: 
> 
> The order of their listing here matches their ordering, when present, in a display set: 
> display definition segment ... 
> page composition segment ... 
> region composition segment ... 
> disparity signalling segment ... 
> CLUT definition segment ... 
> alternative CLUT segment ... 
> object data segment ... 
> end of display set segment ...
> 
> It makes sense for this order to be implemented by ffmpeg.
> 
> I attached a patch, made using git format-patch.

This should really include a micro library version bump.

Thank you, Carl Eugen

Comments

Andriy Gelman Feb. 18, 2020, 8:37 p.m. UTC | #1 
On Tue, 18. Feb 11:27, Alen Vrečko wrote:
> On https://patchwork.ffmpeg.org/project/ffmpeg/list/ the new patch doesn't show up. I used the same filename as before.
> 
> Just in case sending it again with changed filename (and also changed commit message).
> 

Hello Alen,

New patches are ignored by patchwork if the subject line contains "Re:"

I removed the prefix and added a "v2". The link to your updated patch is
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1901993192.202425.1582021627225.JavaMail.zimbra@fora.si/

In the future, you could use git send-email which properly formats the patch.
(you could always test by addressing the patch to yourself)

Also try to avoid top-posting :) It makes the email thread more difficult to
follow.

Thanks,
diff mbox series

Patch

From d3c475fa7ea0984b7f603b02d32eb59363f727f5 Mon Sep 17 00:00:00 2001
From: Alen Vrecko <alen.vrecko@fora.si>
Date: Mon, 17 Feb 2020 15:47:05 +0100
Subject: [PATCH] Change Dvb segment order to be in line with ETSI EN 300 743.

Having CLUT before Region Composition Segment caused problems on legacy STBs i.e. Amino A110.

In the ETSI EN 300 743 V1.6.1 (2018-10) section 4.4 it is written:

The order of their listing here matches their ordering, when present, in a display set:
display definition segment ...
page composition segment ...
region composition segment ...
disparity signalling segment ...
CLUT definition segment ...
alternative CLUT segment ...
object data segment ...
end of display set segment ...

It makes sense for this order to be respected by ffmpeg.
---
 libavcodec/dvbsub.c  | 80 ++++++++++++++++++++++----------------------
 libavcodec/version.h |  2 +-
 2 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/libavcodec/dvbsub.c b/libavcodec/dvbsub.c
index a8d43d81d6..e76d3044b8 100644
--- a/libavcodec/dvbsub.c
+++ b/libavcodec/dvbsub.c
@@ -296,6 +296,46 @@  static int encode_dvb_subtitles(AVCodecContext *avctx,
 
     bytestream_put_be16(&pseg_len, q - pseg_len - 2);
 
+    for (region_id = 0; region_id < h->num_rects; region_id++) {
+
+        /* region composition segment */
+
+        if (h->rects[region_id]->nb_colors <= 4) {
+            /* 2 bpp, some decoders do not support it correctly */
+            bpp_index = 0;
+        } else if (h->rects[region_id]->nb_colors <= 16) {
+            /* 4 bpp, standard encoding */
+            bpp_index = 1;
+        } else if (h->rects[region_id]->nb_colors <= 256) {
+            /* 8 bpp, standard encoding */
+            bpp_index = 2;
+        } else {
+            return -1;
+        }
+
+        *q++ = 0x0f; /* sync_byte */
+        *q++ = 0x11; /* segment_type */
+        bytestream_put_be16(&q, page_id);
+        pseg_len = q;
+        q += 2; /* segment length */
+        *q++ = region_id;
+        *q++ = (s->object_version << 4) | (0 << 3) | 0x07; /* version , no fill */
+        bytestream_put_be16(&q, h->rects[region_id]->w); /* region width */
+        bytestream_put_be16(&q, h->rects[region_id]->h); /* region height */
+        *q++ = ((1 + bpp_index) << 5) | ((1 + bpp_index) << 2) | 0x03;
+        *q++ = region_id; /* clut_id == region_id */
+        *q++ = 0; /* 8 bit fill colors */
+        *q++ = 0x03; /* 4 bit and 2 bit fill colors */
+
+        bytestream_put_be16(&q, region_id); /* object_id == region_id */
+        *q++ = (0 << 6) | (0 << 4);
+        *q++ = 0;
+        *q++ = 0xf0;
+        *q++ = 0;
+
+        bytestream_put_be16(&pseg_len, q - pseg_len - 2);
+    }
+
     if (h->num_rects) {
         for (clut_id = 0; clut_id < h->num_rects; clut_id++) {
 
@@ -346,46 +386,6 @@  static int encode_dvb_subtitles(AVCodecContext *avctx,
         }
     }
 
-    for (region_id = 0; region_id < h->num_rects; region_id++) {
-
-        /* region composition segment */
-
-        if (h->rects[region_id]->nb_colors <= 4) {
-            /* 2 bpp, some decoders do not support it correctly */
-            bpp_index = 0;
-        } else if (h->rects[region_id]->nb_colors <= 16) {
-            /* 4 bpp, standard encoding */
-            bpp_index = 1;
-        } else if (h->rects[region_id]->nb_colors <= 256) {
-            /* 8 bpp, standard encoding */
-            bpp_index = 2;
-        } else {
-            return -1;
-        }
-
-        *q++ = 0x0f; /* sync_byte */
-        *q++ = 0x11; /* segment_type */
-        bytestream_put_be16(&q, page_id);
-        pseg_len = q;
-        q += 2; /* segment length */
-        *q++ = region_id;
-        *q++ = (s->object_version << 4) | (0 << 3) | 0x07; /* version , no fill */
-        bytestream_put_be16(&q, h->rects[region_id]->w); /* region width */
-        bytestream_put_be16(&q, h->rects[region_id]->h); /* region height */
-        *q++ = ((1 + bpp_index) << 5) | ((1 + bpp_index) << 2) | 0x03;
-        *q++ = region_id; /* clut_id == region_id */
-        *q++ = 0; /* 8 bit fill colors */
-        *q++ = 0x03; /* 4 bit and 2 bit fill colors */
-
-        bytestream_put_be16(&q, region_id); /* object_id == region_id */
-        *q++ = (0 << 6) | (0 << 4);
-        *q++ = 0;
-        *q++ = 0xf0;
-        *q++ = 0;
-
-        bytestream_put_be16(&pseg_len, q - pseg_len - 2);
-    }
-
     if (h->num_rects) {
 
         for (object_id = 0; object_id < h->num_rects; object_id++) {
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 5c8147f681..5fff689eb4 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -29,7 +29,7 @@ 
 
 #define LIBAVCODEC_VERSION_MAJOR  58
 #define LIBAVCODEC_VERSION_MINOR  70
-#define LIBAVCODEC_VERSION_MICRO 100
+#define LIBAVCODEC_VERSION_MICRO 101
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
                                                LIBAVCODEC_VERSION_MINOR, \
-- 
2.21.1