ogg dev - Nov 2008 - [PATCH] liboggz: Fix Dirac bitstream parsing

Quite a while ago, the specification was modified to change the
way the nondefault scan format was signalled; this patch fixes that.

Index: liboggz/dirac.c
==================================================================---
liboggz/dirac.c	(revision 3759)
+++ liboggz/dirac.c	(working copy)
@@ -122,6 +122,10 @@
     1, 9, 10, 9, 10, 9, 10, 4, 3, 7, 6, 4, 3, 7, 6, 2, 2, 7, 6, 7, 6,
   };
 
+  static const int dirac_top_field_first[] = { /* extracted from table C.1 */
+    0, 0, 1, 0, 1, 0, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+  };
+
   static const struct {
     ogg_uint32_t width, height;
   } dirac_fsize_tbl[] = { /* table 10.3 framesize */
@@ -150,11 +154,12 @@
   if (dirac_bool( &bs )) {
     info->chroma_format = dirac_uint( &bs ); /* chroma_format */
   }
+
   if (dirac_bool( &bs )) {
-    info->interlaced = 0;
-    if (dirac_bool( &bs )) { /* interlaced */
-        info->interlaced = 1;
-        info->top_field_first = dirac_bool( &bs ); /* top_field_first */
+    int i = dirac_uint( &bs ); /* scan_format */
+    if (i < 2) {
+        info->interlaced = i;
+        info->top_field_first = dirac_top_field_first[video_format];
     }
   }

On Tue, Nov 4, 2008 at 12:56 AM, David Flynn <davidf+nntp at woaf.net>
wrote:
> Quite a while ago, the specification was modified to change the
> way the nondefault scan format was signalled; this patch fixes that.
Thanks for the patch. I've tried to fix things as discussed on irc.
Please review r3762.

Index: /liboggz/trunk/src/liboggz/dirac.c
==================================================================---
/liboggz/trunk/src/liboggz/dirac.c (revision 3665)
+++ /liboggz/trunk/src/liboggz/dirac.c (revision 3762)
@@ -116,9 +116,16 @@
     {1,1}, /* this first value is never used */
     {24000,1001}, {24,1}, {25,1}, {30000,1001}, {30,1},
-    {50,1}, {60000,1001}, {60,1}, {15000,1001}, {25,2},
+    {50,1}, {60000,1001}, {60,1}, {15000,1001}, {25,2}
   };

   static const ogg_uint32_t dirac_vidfmt_frate[] = { /* table C.1 */
-    1, 9, 10, 9, 10, 9, 10, 4, 3, 7, 6, 4, 3, 7, 6, 2, 2, 7, 6, 7, 6,
+    1, 9, 10, 9, 10, 9, 10, 4, 3, 7, 6, 4, 3, 7, 6, 2, 2, 7, 6, 7, 6
+  };
+
+  static const int dirac_source_sampling[] = { /* extracted from table C.1 */
+    0, 0, 0, 0, 0, 0, 0, 1, 1, 0, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0
+  };
+  static const int dirac_top_field_first[] = { /* from table C.1 */
+    0, 0, 1, 0, 1, 0, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1
   };

@@ -151,11 +158,17 @@
     info->chroma_format = dirac_uint( &bs ); /* chroma_format */
   }
-  if (dirac_bool( &bs )) {
-    info->interlaced = 0;
-    if (dirac_bool( &bs )) { /* interlaced */
-        info->interlaced = 1;
-        info->top_field_first = dirac_bool( &bs ); /* top_field_first */
+
+  if (dirac_bool( &bs )) { /* custom_scan_format_flag */
+    int scan_format = dirac_uint( &bs ); /* scan_format */
+    if (scan_format < 2) {
+      info->interlaced = scan_format;
+    } else { /* other scan_format values are reserved */
+      info->interlaced = 0;
     }
+  } else { /* no custom scan_format, use the preset value */
+    info->interlaced = dirac_source_sampling[video_format];
   }
+  /* field order is set by video_format and cannot be custom */
+  info->top_field_first = dirac_top_field_first[video_format];

   info->fps_numerator
dirac_frate_tbl[dirac_vidfmt_frate[video_format]].fps_numerator;

On 2008-11-04, Ralph Giles <giles at xiph.org>
wrote:
> On Tue, Nov 4, 2008 at 12:56 AM, David Flynn <davidf+nntp at
woaf.net> wrote:
>
>> Quite a while ago, the specification was modified to change the
>> way the nondefault scan format was signalled; this patch fixes that.
>
> Thanks for the patch. I've tried to fix things as discussed on irc.
> Please review r3762.
That looks fine.

..david
> -    {50,1}, {60000,1001}, {60,1}, {15000,1001}, {25,2},
> +    {50,1}, {60000,1001}, {60,1}, {15000,1001}, {25,2}
Interesting -- i usually leave the final `,' there so that any future
patch that appends something to the list doesn't have to modify the
previous line to insert the ','

..david

On Tue, Nov 4, 2008 at 2:41 PM, David Flynn <davidf+nntp at woaf.net>
wrote:
>> -    {50,1}, {60000,1001}, {60,1}, {15000,1001}, {25,2},
>> +    {50,1}, {60000,1001}, {60,1}, {15000,1001}, {25,2}
>
> Interesting -- i usually leave the final `,' there so that any future
> patch that appends something to the list doesn't have to modify the
> previous line to insert the ','
Good point about patches. I just though it was for editing
convenience. I've been broken of that by (ancient) compilers who are
confused by trailing commas. The file was inconsistent so I normalized
it in favour of my personal habit.

For liboggz I don't think it's important: people tend not to build
media software on such machines. So we can put them back if you want.

 -r