WebVTT module does not parent tags correctly

Sample VTT file

Added attachment:

blank.vtt

Screenshot of erroneous styling

Added attachment:

Screenshot 2019-09-27 at 01.51.51.png

Screenshot of corrected styling after said code modification

Added attachment:

Screenshot 2019-09-27 at 02.07.13.png

Just want to add that node parents are used to determine style in a cascading way, and this is why node hierarchy impacts rendering... if that wasn't clear.

Fix is incorrect. Already creating the nodes tree properly.

CUE 
 TEXT iub 
 TAGu ((null))
  TEXT iUb 
  TAGi ((null))
   TEXT IUb 
   TAGb ((null))
    TEXT IUB
   TEXT  IUb
  TEXT  iUb
 TEXT  iub

That's right, the tree is displayed correctly by the debug function, and that's why it took me forever to track down the issue...

The problem lays in the fact that the segment that follows a closing tag doesn't get the correct parent (despite the fact that it is indented properly in the debug function). Because the ComputeStyle() function applies styles in a cascading way (and it should), the identity of the parent matters there. What happens is that the leftover segment (that is, here, the second "TEXT IUb") gets the previous node as a parent (here the b tag).

I'm traveling and I don't have my test code with me to try again, but I had determined that this happened because of the if(p_parent) on line 1254. If p_parent is p_head and p_head is initialized to NULL, the having no parent (being head) and having head as a parent cannot be distinguished.

I'll look more in detail, and try to see why the debug function also shows the tree properly, as I don't understand it fully... but perhaps in the meantime you can try to compile with the proposed correction and see for yourself if it fixes the problem or not?

So first, the webvtt_domnode_Debug function doesn't check what parent is what it just uses child and next and increases indentation recursively. Maybe a way to convince yourself of the problem is to also display the parent (as an address) of each node.

I guess it would be something like this:

static void webvtt_domnode_Debug( webvtt_dom_node_t *p_node, int i_depth )
{
    for( ; p_node ; p_node = p_node->p_next )
    {
        for( int i=0; i<i_depth; i++) printf(" ");
        if( p_node->type == NODE_TEXT )
        {
            printf("TEXT %s -> parent=%p\n", ((webvtt_dom_text_t *)p_node)->psz_text, (void *)(p_node->p_parent) );
        }
        else if( p_node->type == NODE_TAG )
        {
            webvtt_dom_tag_t *p_tag = (webvtt_dom_tag_t *)p_node;
            printf("TAG%s (%s) -> parent=%p\n", p_tag->psz_tag, p_tag->psz_attrs, (void *)(p_tag->p_parent) );
            webvtt_domnode_Debug( p_tag->p_child, i_depth + 1 );
        }
        else if( p_node->type == NODE_CUE )
        {
            webvtt_dom_cue_t *p_cue = (webvtt_dom_cue_t *)p_node;
            printf("CUE %s\n", p_cue->psz_id );
            webvtt_domnode_Debug( p_cue->p_child, i_depth + 1 );
        }
        else if( p_node->type == NODE_REGION )
        {
            webvtt_region_t *p_region = (webvtt_region_t *)p_node;
            printf("REGION %s\n", p_region->psz_id );
            webvtt_domnode_Debug( p_region->p_child, i_depth + 1 );
        }
    }
}

Hopefully that'll show that the parent of the leftover segment is incorrect.

Proper fix is

diff --git a/modules/codec/webvtt/subsvtt.c b/modules/codec/webvtt/subsvtt.c
index b432918d3d..ab01d40907 100644
--- a/modules/codec/webvtt/subsvtt.c
+++ b/modules/codec/webvtt/subsvtt.c
@@ -1262,7 +1262,10 @@ static webvtt_dom_node_t * CreateDomNodes( const char *psz_text, unsigned *pi_li
                      * like <b><v stuff>foo</b> */
                     p_parent = webvtt_domnode_getParentByTag( p_parent, psz_tagname );
                     if( p_parent ) /* continue as parent next */
+                    {
                         pp_append = &p_parent->p_next;
+                        p_parent = p_parent->p_parent;
+                    }
                     else /* back as top node */
                         pp_append = &p_head->p_next;
                     while( *pp_append )

however, it trigger some bugs

Yes! You're right! I may have had this in my code too... and that may be why I ended up allocating p_head, so that p_head's parent is NULL, but p_head's children can still have p_head as a parent... or am I just blabbering at that point...?