openlp-core team mailing list archive

[Andreas Preikschat <andreaspreikschat@xxxxxxxxxx>]

Andreas Preikschat has proposed merging lp:~googol/openlp/render into lp:openlp.

Requested reviews:
  Tim Bentley (trb143)
  Raoul Snyman (raoul-snyman)

For more details, see:
https://code.launchpad.net/~googol/openlp/render/+merge/66253

Hello,

1) improved rendering speed
My assumption was, that the majority of verses/chorus will fit on a slide. That is why we first check if the verse/chorus fits as a whole and if it does we do not do any further rendering. For those verses/choruses the rendering is very fast because we need to load the slide text only once.
If it does not fit, then we use the binary chop algorithm to process the slide text.

2) clean ups/simplifications
- do not use instance variables as arguments
- renamed image_manager to imageManager and theme_manager to themeManager as the class is based on Qt

Concrete changes I made:
- moved the binary chop algorithm to its own method
- made the binary chop less 'word by word' specific (line ending/separator)
- Improved "detection" when to use which method to render an item. (The _paginate_slide_words is able to render the text without help, thus we do not need to render the text first with _paginate_slide and then *again* with _paginate_slide_words.)

Test results:
As test data I chose the song "Amazing Grace" and "Esther 7-8". As themes I used three themes (based on "Sea with tress" with 40, 50 and 65pt as main font size).

I created a service file for you (http://ubuntuone.com/p/yyM/) My test items were:
Amazing Grace 40
Amazing Grace 50
Amazing Grace 65
Esther 7-8 (Verse per Slide)
Esther 7-8 (Verse per Line)
Esther 7-8 (Continuous)

The numbers stand for the main font size (theme) they use. "Amazing Grace 65" does not have verse 6 (instead verse 5 and verse 6 were merged and separated with a soft break). Bibles verses have been rendered with 65pt.

I enabled the service file auto load and measured the times (time speed in the (outer) for loop in the render method in serviceitem.py). Each result represents the time needed for one item.

branch (average of two)
0.174 <- Amazing Grace 40
0.314 <- Amazing Grace 50
0.462 <- Amazing Grace 65
4.431 <- Esther 7-8 (Verse per Slide)
3.490 <- Esther 7-8 (Verse per Line)
3.496 <- Esther 7-8 (Continuous)

trunk (average of two)
0.276
0.555
0.708
4.615
4.353
4.490
-- 
https://code.launchpad.net/~googol/openlp/render/+merge/66253
Your team OpenLP Core is subscribed to branch lp:openlp.

=== modified file 'openlp/core/lib/__init__.py'
--- openlp/core/lib/__init__.py	2011-06-12 16:02:52 +0000
+++ openlp/core/lib/__init__.py	2011-06-29 07:49:27 +0000
@@ -202,7 +202,7 @@
     """
     Remove Tags from text for display
     """
-    text = text.replace(u'<br>', u'\n')
+    text = text.replace(u'<br />', u'\n')
     text = text.replace(u'{br}', u'\n')
     text = text.replace(u'&nbsp;', u' ')
     for tag in DisplayTags.get_html_tags():

=== modified file 'openlp/core/lib/displaytags.py'
--- openlp/core/lib/displaytags.py	2011-06-12 16:02:52 +0000
+++ openlp/core/lib/displaytags.py	2011-06-29 07:49:27 +0000
@@ -111,7 +111,7 @@
             u'start html': u'<span style="text-decoration: underline;">',
             u'end tag': u'{/u}', u'end html': u'</span>', u'protected': True})
         base_tags.append({u'desc': translate('OpenLP.DisplayTags', 'Break'),
-            u'start tag': u'{br}', u'start html': u'<br>', u'end tag': u'',
+            u'start tag': u'{br}', u'start html': u'<br />', u'end tag': u'',
             u'end html': u'', u'protected': True})
         DisplayTags.add_html_tags(base_tags)
 

=== modified file 'openlp/core/lib/renderer.py'
--- openlp/core/lib/renderer.py	2011-06-12 16:02:52 +0000
+++ openlp/core/lib/renderer.py	2011-06-29 07:49:27 +0000
@@ -56,20 +56,20 @@
     """
     log.info(u'Renderer Loaded')
 
-    def __init__(self, image_manager, theme_manager):
+    def __init__(self, imageManager, themeManager):
         """
         Initialise the render manager.
 
-    ``image_manager``
+    ``imageManager``
         A ImageManager instance which takes care of e. g. caching and resizing
         images.
 
-    ``theme_manager``
+    ``themeManager``
         The ThemeManager instance, used to get the current theme details.
         """
         log.debug(u'Initialisation started')
-        self.theme_manager = theme_manager
-        self.image_manager = image_manager
+        self.themeManager = themeManager
+        self.imageManager = imageManager
         self.screens = ScreenList.get_instance()
         self.service_theme = u''
         self.theme_level = u''
@@ -77,7 +77,7 @@
         self.theme_data = None
         self.bg_frame = None
         self.force_page = False
-        self.display = MainDisplay(None, self.image_manager, False)
+        self.display = MainDisplay(None, self.imageManager, False)
         self.display.setup()
 
     def update_display(self):
@@ -88,7 +88,7 @@
         self._calculate_default(self.screens.current[u'size'])
         if self.display:
             self.display.close()
-        self.display = MainDisplay(None, self.image_manager, False)
+        self.display = MainDisplay(None, self.imageManager, False)
         self.display.setup()
         self.bg_frame = None
         self.theme_data = None
@@ -101,14 +101,14 @@
             The global-level theme to be set.
 
         ``theme_level``
-            Defaults to *``ThemeLevel.Global``*. The theme level, can be
+            Defaults to ``ThemeLevel.Global``. The theme level, can be
             ``ThemeLevel.Global``, ``ThemeLevel.Service`` or
             ``ThemeLevel.Song``.
         """
         self.global_theme = global_theme
         self.theme_level = theme_level
         self.global_theme_data = \
-            self.theme_manager.getThemeData(self.global_theme)
+            self.themeManager.getThemeData(self.global_theme)
         self.theme_data = None
 
     def set_service_theme(self, service_theme):
@@ -162,12 +162,12 @@
         if override_levels:
             self.theme_data = override_theme
         else:
-            self.theme_data = self.theme_manager.getThemeData(theme)
+            self.theme_data = self.themeManager.getThemeData(theme)
         self._calculate_default(self.screens.current[u'size'])
         self._build_text_rectangle(self.theme_data)
         # if No file do not update cache
         if self.theme_data.background_filename:
-            self.image_manager.add_image(self.theme_data.theme_name,
+            self.imageManager.add_image(self.theme_data.theme_name,
                 self.theme_data.background_filename)
         return self._rect, self._rect_footer
 
@@ -193,7 +193,7 @@
             # make big page for theme edit dialog to get line count
             serviceItem.add_from_text(u'', VERSE + VERSE + VERSE)
         else:
-            self.image_manager.del_image(theme_data.theme_name)
+            self.imageManager.del_image(theme_data.theme_name)
             serviceItem.add_from_text(u'', VERSE)
         serviceItem.renderer = self
         serviceItem.raw_footer = FOOTER
@@ -205,43 +205,52 @@
             # Reset the real screen size for subsequent render requests
             self._calculate_default(self.screens.current[u'size'])
             return preview
+        self.force_page = False
 
-    def format_slide(self, text, line_break, item):
+    def format_slide(self, text, item):
         """
         Calculate how much text can fit on a slide.
 
         ``text``
             The words to go on the slides.
 
-        ``line_break``
-            Add line endings after each line of text used for bibles.
+        ``item``
+            The :class:`~openlp.core.lib.serviceitem.ServiceItem` item object.
         """
         log.debug(u'format slide')
-        # clean up line endings
-        lines = self._lines_split(text)
-        pages = self._paginate_slide(lines, line_break, self.force_page)
-        if len(pages) > 1:
-            # Songs and Custom
-            if item.is_capable(ItemCapabilities.AllowsVirtualSplit):
-                # Do not forget the line breaks !
-                slides = text.split(u'[---]')
-                pages = []
-                for slide in slides:
-                    lines = slide.strip(u'\n').split(u'\n')
-                    new_pages = self._paginate_slide(lines, line_break,
-                        self.force_page)
-                    pages.extend(new_pages)
-            # Bibles
-            elif item.is_capable(ItemCapabilities.AllowsWordSplit):
-                pages = self._paginate_slide_words(text, line_break)
-        return pages
+        # Add line endings after each line of text used for bibles.
+        line_end = u'<br />'
+        if item.is_capable(ItemCapabilities.NoLineBreaks):
+            line_end = u' '
+        # Bibles
+        if item.is_capable(ItemCapabilities.AllowsWordSplit):
+            pages = self._paginate_slide_words(text, line_end)
+        else:
+            # Clean up line endings.
+            lines = self._lines_split(text)
+            pages = self._paginate_slide(lines, line_end)
+            if len(pages) > 1:
+                # Songs and Custom
+                if item.is_capable(ItemCapabilities.AllowsVirtualSplit):
+                    # Do not forget the line breaks!
+                    slides = text.split(u'[---]')
+                    pages = []
+                    for slide in slides:
+                        lines = slide.strip(u'\n').split(u'\n')
+                        pages.extend(self._paginate_slide(lines, line_end))
+        new_pages = []
+        for page in pages:
+            while page.endswith(u'<br />'):
+                page = page[:-6]
+            new_pages.append(page)
+        return new_pages
 
     def _calculate_default(self, screen):
         """
         Calculate the default dimentions of the screen.
 
         ``screen``
-            The QSize of the screen.
+            The screen to calculate the default of.
         """
         log.debug(u'_calculate default %s', screen)
         self.width = screen.width()
@@ -308,54 +317,37 @@
             (build_lyrics_format_css(self.theme_data, self.page_width,
             self.page_height), build_lyrics_outline_css(self.theme_data))
 
-    def _paginate_slide(self, lines, line_break, force_page=False):
+    def _paginate_slide(self, lines, line_end):
         """
         Figure out how much text can appear on a slide, using the current
         theme settings.
 
         ``lines``
-            The words to be fitted on the slide split into lines.
-
-        ``line_break``
-            Add line endings after each line of text (used for bibles).
-
-        ``force_page``
-            Flag to tell message lines in page.
-
+            The text to be fitted on the slide split into lines.
+
+        ``line_end``
+            The text added after each line. Either ``u' '`` or ``u'<br />``.
         """
         log.debug(u'_paginate_slide - Start')
-        line_end = u''
-        if line_break:
-            line_end = u'<br>'
         formatted = []
-        html_text = u''
-        styled_text = u''
-        line_count = 0
-        for line in lines:
-            if line_count != -1:
-                line_count += 1
-            styled_line = expand_tags(line) + line_end
-            styled_text += styled_line
-            html = self.page_shell + styled_text + HTML_END
-            self.web.setHtml(html)
-            # Text too long so go to next page.
-            if self.web_frame.contentsSize().height() > self.page_height:
-                if force_page and line_count > 0:
-                    Receiver.send_message(u'theme_line_count', line_count - 1)
-                line_count = -1
-                while html_text.endswith(u'<br>'):
-                    html_text = html_text[:-4]
-                formatted.append(html_text)
-                html_text = u''
-                styled_text = styled_line
-            html_text += line + line_end
-        while html_text.endswith(u'<br>'):
-            html_text = html_text[:-4]
-        formatted.append(html_text)
+        previous_html = u''
+        previous_raw = u''
+        separator = u'<br />'
+        html_lines = map(expand_tags, lines)
+        html = self.page_shell + separator.join(html_lines) + HTML_END
+        self.web.setHtml(html)
+        # Text too long so go to next page.
+        if self.web_frame.contentsSize().height() > self.page_height:
+            html_text, previous_raw = self._binary_chop(formatted,
+                previous_html, previous_raw, html_lines, lines, separator, u'')
+        else:
+            previous_raw = separator.join(lines)
+        if previous_raw:
+            formatted.append(previous_raw)
         log.debug(u'_paginate_slide - End')
         return formatted
 
-    def _paginate_slide_words(self, text, line_break):
+    def _paginate_slide_words(self, text, line_end):
         """
         Figure out how much text can appear on a slide, using the current
         theme settings. This version is to handle text which needs to be split
@@ -364,22 +356,19 @@
         ``text``
             The words to be fitted on the slide split into lines.
 
-        ``line_break``
-            Add line endings after each line of text used for bibles.
-
+        ``line_end``
+            The text added after each line. Either ``u' '`` or ``u'<br />``.
+            This is needed for bibles.
         """
         log.debug(u'_paginate_slide_words - Start')
-        line_end = u' '
-        if line_break:
-            line_end = u'<br>'
         formatted = []
         previous_html = u''
         previous_raw = u''
         lines = text.split(u'\n')
         for line in lines:
             line = line.strip()
-            styled_line = expand_tags(line)
-            html = self.page_shell + previous_html + styled_line + HTML_END
+            html_line = expand_tags(line)
+            html = self.page_shell + previous_html + html_line + HTML_END
             self.web.setHtml(html)
             # Text too long so go to next page.
             if self.web_frame.contentsSize().height() > self.page_height:
@@ -390,88 +379,123 @@
                     self.web.setHtml(html)
                     if self.web_frame.contentsSize().height() <= \
                         self.page_height:
-                        while previous_raw.endswith(u'<br>'):
-                            previous_raw = previous_raw[:-4]
                         formatted.append(previous_raw)
                         previous_html = u''
                         previous_raw = u''
-                        html = self.page_shell + styled_line + HTML_END
+                        html = self.page_shell + html_line + HTML_END
                         self.web.setHtml(html)
                         # Now check if the current verse will fit, if it does
                         # not we have to start to process the verse word by
                         # word.
                         if self.web_frame.contentsSize().height() <= \
                             self.page_height:
-                            previous_html = styled_line + line_end
+                            previous_html = html_line + line_end
                             previous_raw = line + line_end
                             continue
-                # Figure out how many words of the line will fit on screen by
-                # using the algorithm known as "binary chop".
+                # Figure out how many words of the line will fit on screen as
+                # the line will not fit as a whole.
                 raw_words = self._words_split(line)
-                html_words = [expand_tags(word) for word in raw_words]
-                smallest_index = 0
-                highest_index = len(html_words) - 1
-                index = int(highest_index / 2)
-                while True:
-                    html = self.page_shell + previous_html + \
-                        u''.join(html_words[:index + 1]).strip() + HTML_END
-                    self.web.setHtml(html)
-                    if self.web_frame.contentsSize().height() > \
-                        self.page_height:
-                        # We know that it does not fit, so change/calculate the
-                        # new index and highest_index accordingly.
-                        highest_index = index
-                        index = int(index - (index - smallest_index) / 2)
-                    else:
-                        smallest_index = index
-                        index = int(index + (highest_index - index) / 2)
-                    # We found the number of words which will fit.
-                    if smallest_index == index or highest_index == index:
-                        index = smallest_index
-                        formatted.append(previous_raw.rstrip(u'<br>') +
-                            u''.join(raw_words[:index + 1]))
-                        previous_html = u''
-                        previous_raw = u''
-                    else:
-                        continue
-                    # Check if the rest of the line fits on the slide. If it
-                    # does we do not have to do the much more intensive "word by
-                    # word" checking.
-                    html = self.page_shell + \
-                        u''.join(html_words[index + 1:]).strip() + HTML_END
-                    self.web.setHtml(html)
-                    if self.web_frame.contentsSize().height() <= \
-                        self.page_height:
-                        previous_html = \
-                            u''.join(html_words[index + 1:]).strip() + line_end
-                        previous_raw = \
-                            u''.join(raw_words[index + 1:]).strip() + line_end
-                        break
-                    else:
-                        # The other words do not fit, thus reset the indexes,
-                        # create a new list and continue with "word by word".
-                        raw_words = raw_words[index + 1:]
-                        html_words = html_words[index + 1:]
-                        smallest_index = 0
-                        highest_index = len(html_words) - 1
-                        index = int(highest_index / 2)
+                html_words = map(expand_tags, raw_words)
+                previous_html, previous_raw = self._binary_chop(
+                    formatted, previous_html, previous_raw, html_words,
+                    raw_words, u' ', line_end)
             else:
-                previous_html += styled_line + line_end
+                previous_html += html_line + line_end
                 previous_raw += line + line_end
-        while previous_raw.endswith(u'<br>'):
-            previous_raw = previous_raw[:-4]
         formatted.append(previous_raw)
         log.debug(u'_paginate_slide_words - End')
         return formatted
 
+    def _binary_chop(self, formatted, previous_html, previous_raw, html_list,
+        raw_list, separator, line_end):
+        """
+        This implements the binary chop algorithm for faster rendering. This
+        algorithm works line based (line by line) and word based (word by word).
+        It is assumed that this method is **only** called, when the lines/words
+        to be rendered do **not** fit as a whole.
+
+        ``formatted``
+            The list to append any slides.
+
+        ``previous_html``
+            The html text which is know to fit on a slide, but is not yet added
+            to the list of slides. (unicode string)
+
+        ``previous_raw``
+            The raw text (with display tags) which is know to fit on a slide,
+            but is not yet added to the list of slides. (unicode string)
+
+        ``html_list``
+            The elements which do not fit on a slide and needs to be processed
+            using the binary chop. The text contains html.
+
+        ``raw_list``
+            The elements which do not fit on a slide and needs to be processed
+            using the binary chop. The elements can contain display tags.
+
+        ``separator``
+            The separator for the elements. For lines this is ``u'<br />'`` and
+            for words this is ``u' '``.
+
+        ``line_end``
+            The text added after each "element line". Either ``u' '`` or
+            ``u'<br />``. This is needed for bibles.
+        """
+        smallest_index = 0
+        highest_index = len(html_list) - 1
+        index = int(highest_index / 2)
+        while True:
+            html = self.page_shell + previous_html + \
+                separator.join(html_list[:index + 1]).strip() + HTML_END
+            self.web.setHtml(html)
+            if self.web_frame.contentsSize().height() > self.page_height:
+                # We know that it does not fit, so change/calculate the
+                # new index and highest_index accordingly.
+                highest_index = index
+                index = int(index - (index - smallest_index) / 2)
+            else:
+                smallest_index = index
+                index = int(index + (highest_index - index) / 2)
+            # We found the number of words which will fit.
+            if smallest_index == index or highest_index == index:
+                index = smallest_index
+                formatted.append(previous_raw.rstrip(u'<br />') +
+                    separator.join(raw_list[:index + 1]))
+                previous_html = u''
+                previous_raw = u''
+                # Stop here as the theme line count was requested.
+                if self.force_page:
+                    Receiver.send_message(u'theme_line_count', index + 1)
+                    break
+            else:
+                continue
+            # Check if the remaining elements fit on the slide.
+            html = self.page_shell + \
+                separator.join(html_list[index + 1:]).strip() + HTML_END
+            self.web.setHtml(html)
+            if self.web_frame.contentsSize().height() <= self.page_height:
+                previous_html = separator.join(
+                    html_list[index + 1:]).strip() + line_end
+                previous_raw = separator.join(
+                    raw_list[index + 1:]).strip() + line_end
+                break
+            else:
+                # The remaining elements do not fit, thus reset the indexes,
+                # create a new list and continue.
+                raw_list = raw_list[index + 1:]
+                html_list = html_list[index + 1:]
+                smallest_index = 0
+                highest_index = len(html_list) - 1
+                index = int(highest_index / 2)
+        return previous_html, previous_raw
+
     def _words_split(self, line):
         """
         Split the slide up by word so can wrap better
         """
         # this parse we are to be wordy
         line = line.replace(u'\n', u' ')
-        words = line.split(u' ')
-        return [word + u' ' for word in words]
+        return line.split(u' ')
 
     def _lines_split(self, text):
         """
@@ -479,5 +503,5 @@
         """
         # this parse we do not want to use this so remove it
         text = text.replace(u'\n[---]', u'')
-        lines = text.split(u'\n')
-        return [line.replace(u'[---]', u'') for line in lines]
+        text = text.replace(u'[---]', u'')
+        return text.split(u'\n')

=== modified file 'openlp/core/lib/serviceitem.py'
--- openlp/core/lib/serviceitem.py	2011-06-18 13:36:29 +0000
+++ openlp/core/lib/serviceitem.py	2011-06-29 07:49:27 +0000
@@ -165,7 +165,6 @@
         log.debug(u'Render called')
         self._display_frames = []
         self.bg_image_bytes = None
-        line_break = not self.is_capable(ItemCapabilities.NoLineBreaks)
         theme = self.theme if self.theme else None
         self.main, self.footer = \
             self.renderer.set_override_theme(theme, use_override)
@@ -173,10 +172,9 @@
         if self.service_item_type == ServiceItemType.Text:
             log.debug(u'Formatting slides')
             for slide in self._raw_frames:
-                formatted = self.renderer \
-                    .format_slide(slide[u'raw_slide'], line_break, self)
-                for page in formatted:
-                    page = page.replace(u'<br>', u'{br}')
+                pages = self.renderer.format_slide(slide[u'raw_slide'], self)
+                for page in pages:
+                    page = page.replace(u'<br />', u'{br}')
                     html = expand_tags(cgi.escape(page.rstrip()))
                     self._display_frames.append({
                         u'title': clean_tags(page),
@@ -196,7 +194,7 @@
         if self.raw_footer is None:
             self.raw_footer = []
         self.foot_text = \
-            u'<br>'.join([footer for footer in self.raw_footer if footer])
+            u'<br />'.join([footer for footer in self.raw_footer if footer])
 
     def add_from_image(self, path, title):
         """
@@ -210,7 +208,7 @@
         """
         self.service_item_type = ServiceItemType.Image
         self._raw_frames.append({u'title': title, u'path': path})
-        self.renderer.image_manager.add_image(title, path)
+        self.renderer.imageManager.add_image(title, path)
         self._new_item()
 
     def add_from_text(self, title, raw_slide, verse_tag=None):

=== modified file 'openlp/core/ui/maindisplay.py'
--- openlp/core/ui/maindisplay.py	2011-06-21 21:08:30 +0000
+++ openlp/core/ui/maindisplay.py	2011-06-29 07:49:27 +0000
@@ -48,13 +48,13 @@
     """
     This is the display screen.
     """
-    def __init__(self, parent, image_manager, live):
+    def __init__(self, parent, imageManager, live):
         if live:
             QtGui.QGraphicsView.__init__(self)
         else:
             QtGui.QGraphicsView.__init__(self, parent)
         self.isLive = live
-        self.image_manager = image_manager
+        self.imageManager = imageManager
         self.screens = ScreenList.get_instance()
         self.alertTab = None
         self.hideMode = None
@@ -232,7 +232,7 @@
         """
         API for replacement backgrounds so Images are added directly to cache
         """
-        self.image_manager.add_image(name, path)
+        self.imageManager.add_image(name, path)
         self.image(name)
         if hasattr(self, u'serviceItem'):
             self.override[u'image'] = name
@@ -247,7 +247,7 @@
             The name of the image to be displayed
         """
         log.debug(u'image to display')
-        image = self.image_manager.get_image_bytes(name)
+        image = self.imageManager.get_image_bytes(name)
         self.resetVideo()
         self.displayImage(image)
         return self.preview()
@@ -477,13 +477,13 @@
                 self.override = {}
             else:
                 # replace the background
-                background = self.image_manager. \
+                background = self.imageManager. \
                     get_image_bytes(self.override[u'image'])
         if self.serviceItem.themedata.background_filename:
-            self.serviceItem.bg_image_bytes = self.image_manager. \
+            self.serviceItem.bg_image_bytes = self.imageManager. \
                 get_image_bytes(self.serviceItem.themedata.theme_name)
         if image:
-            image_bytes = self.image_manager.get_image_bytes(image)
+            image_bytes = self.imageManager.get_image_bytes(image)
         else:
             image_bytes = None
         html = build_html(self.serviceItem, self.screen, self.alertTab,

=== modified file 'openlp/plugins/songs/forms/editsongform.py'
--- openlp/plugins/songs/forms/editsongform.py	2011-06-23 13:27:06 +0000
+++ openlp/plugins/songs/forms/editsongform.py	2011-06-29 07:49:27 +0000
@@ -90,7 +90,7 @@
             self.onVerseListViewPressed)
         QtCore.QObject.connect(self.themeAddButton,
             QtCore.SIGNAL(u'clicked()'),
-            self.mediaitem.plugin.renderer.theme_manager.onAddTheme)
+            self.mediaitem.plugin.renderer.themeManager.onAddTheme)
         QtCore.QObject.connect(self.maintenanceButton,
             QtCore.SIGNAL(u'clicked()'), self.onMaintenanceButtonClicked)
         QtCore.QObject.connect(Receiver.get_receiver(),